-
Notifications
You must be signed in to change notification settings - Fork 760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds tetr.io support #43
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few questions and thoughts below, but I'm not sure we need a code change here.
It seems like the issue is that tetr.io requires https, but this is only a http server. So I'm a little worried that this might break the proxy for some deployments.
To use https with a http server like this one, you need a reverse proxy that handles https terminition between the node.js server and the internet. I'm guessing you have that since this apparently worked for you.
I use nginx proxy manager for https termination, and it has an option to force requests to be https. Enabling that option would have the same effect as this PR without needing a code change and without the possible side effect of breaking it for deployments without https.
function forceUpgrade(html) { | ||
var meta = [ | ||
"<meta http-equiv=\"Content-Security-Policy\" content=\"upgrade-insecure-requests\">" | ||
].join("\n"); | ||
html = html.replace("</head>", meta + "\n\n</head>"); | ||
return html; | ||
} | ||
|
||
function forceHttpsUpgradeMiddleware(data) { | ||
if (data.contentType == 'text/html') { | ||
|
||
// https://nodejs.org/api/stream.html#stream_transform | ||
data.stream = data.stream.pipe(new Transform({ | ||
decodeStrings: false, | ||
transform: function(chunk, encoding, next) { | ||
this.push(forceUpgrade(chunk.toString())); | ||
next(); | ||
} | ||
})); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this code used anywhere, or was it just the initial attempt and can be removed now?
function tetrioPatch(html) { | ||
var meta = [ | ||
"<meta http-equiv=\"Content-Security-Policy\" content=\"upgrade-insecure-requests\">" | ||
].join("\n"); | ||
html = html.replace("<meta name=googlebot content=notranslate>", "<meta name=googlebot content=notranslate>\n\n" + meta); | ||
return html; | ||
} | ||
|
||
function tetrioPatchMiddleware(data) { | ||
if (data.contentType == 'text/html') { | ||
|
||
// https://nodejs.org/api/stream.html#stream_transform | ||
data.stream = data.stream.pipe(new Transform({ | ||
decodeStrings: false, | ||
transform: function(chunk, encoding, next) { | ||
this.push(tetrioPatch(chunk.toString())); | ||
next(); | ||
} | ||
})); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the basic issue that tetr.io requires https, and so this is one way of forcing https?
What does this do if you only have http, and don't have a https server?
Also, if it's specifically for tetr.io, then the if check should be conditioned on that in addition to the contentType, similar to how we check for youtube.com here: https://github.com/nfriedly/node-unblocker/blob/ee8358df24dc6abe2cc4884c48e3d0e44b57c0b2/examples/youtube/youtube.js#L7-L8
Lastly, if we do keep this, I think this should go in a separate file that's just imported into app.js.
"<!-- Google tag (gtag.js) -->", | ||
"<script async src=\"https://www.googletagmanager.com/gtag/js?id=" + google_analytics_id + "\"></script>", | ||
"<script>", | ||
" window.dataLayer = window.dataLayer || [];", | ||
" function gtag(){dataLayer.push(arguments);}", | ||
" gtag('js', new Date());", | ||
"\n", | ||
" gtag('config', " + google_analytics_id + ");", | ||
"</script>" | ||
].join("\n"); | ||
html = html.replace("<head>", "<head>\n\n" + ga); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah, this has been broken for a while hasn't it. I probably would have just removed it, but this is fine too.
Tetr.io is a live competitive Tetris game that did not work with nodeunblocker when I was using it; I added tetr.io support, which was working as of the last time I tested it.