Bug 1313395 - Forward several headers in websocket upgrade requests #6373
Conversation
|
This LGTM but I don't know much about the subject matter. @Miciah are you familiar enough with this to review? |
|
|
||
| /* Set X-Client-IP HTTP extension header. */ | ||
| zheaders.headers['X-Client-IP'] = xff; | ||
|
|
There was a problem hiding this comment.
Does case matter here? We're sometimes using lower-case, sometimes using capitals.
HTTP is supposed to be case-insensitive, the documentation for the http module indicates Node.js normalises headers to lower-case when they are received, and as far as I can tell the http module's accessors do the same, so lower-case seems to be preferred in Node.js (although some examples in the documentation use capitals). However, the documentation for the ws module doesn't say anything at all about case, and we are constructing and passing in the headers object ourselves rather than using any sort of accessor, so I'm wary of making assumptions.
@bparees, does this fall within your expertise?
There was a problem hiding this comment.
not particularly, but i'd second your belief that case is irrelevant, so it's basically aesthetics.
|
openshift-bot, please [merge]! |
|
Online Merge Results: SUCCESS (https://ci.dev.openshift.redhat.com/jenkins/job/merge_pull_requests/6721/) (Image: devenv_5781) |
|
Evaluated for online merge up to bc5f3cc |
No description provided.