Skip to content
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

[WebUI] relax CSRF defense. Closes #6882. #6887

Merged
merged 1 commit into from Jun 13, 2017

Conversation

Chocobo1
Copy link
Member

@Chocobo1 Chocobo1 commented Jun 2, 2017

Allow HTTP request which has neither Origin nor Referer header included.

summary: #6882 (comment)

Allow HTTP request which has neither Origin nor Referer header included
@naikel
Copy link
Contributor

naikel commented Jun 2, 2017

@Chocobo1 can you fix the capitalization of the X-Forwarded-Host header? I think you typed it as "x-forwarded-host". Even though this works fine, it looks weird.

@Chocobo1
Copy link
Member Author

Chocobo1 commented Jun 2, 2017

can you fix the capitalization of the X-Forwarded-Host header?

AFAIK, http header name are case insensitive (RFC 7230?), and we will convert all header name constants to lowercase in near future.

UPDATE:

RFC7230
3.2.  Header Fields
   Each header field consists of a case-insensitive field name followed by a colon (":"),
   optional leading whitespace, the field value, and optional trailing whitespace.

@naikel
Copy link
Contributor

naikel commented Jun 2, 2017

@Chocobo1 Yes, like I said that works fine, but all other headers in the source code are capitalized like the browsers send them:

    const char HEADER_X_CONTENT_TYPE_OPTIONS[] = "X-Content-Type-Options";
    const char HEADER_X_FRAME_OPTIONS[] = "X-Frame-Options";
    const char HEADER_X_XSS_PROTECTION[] = "X-XSS-Protection";

It's just to be consistent with the rest of the code and it looks better.

@Chocobo1
Copy link
Member Author

Chocobo1 commented Jun 2, 2017

It's just to be consistent with the rest of the code and it looks better.

Please... it's just temporal and qbt already stores header name in lowercase, using capitalized string WON'T WORK.

@naikel
Copy link
Contributor

naikel commented Jun 2, 2017

@Chocobo1 It's just for readability of the source code, all other headers are capitalized for readability, it's just readability, it doesn't have anything to do with functionality. Don't you think it would look weird on types.h that every single header but yours is capitalized?

@Chocobo1
Copy link
Member Author

Chocobo1 commented Jun 2, 2017

Don't you think it would look weird on types.h that every single header but yours is capitalized?

If I change all header name constants to lowercase now, that makes you happy?
Serious question.

@naikel
Copy link
Contributor

naikel commented Jun 2, 2017

@Chocobo1 yes, but I feel like you want to go against the world. All browsers send them capitalized. Curl and wget also send them capitalized. They are already capitalized in the source code. I know it makes no difference, but why you want to be different?

    const char HEADER_X_CONTENT_TYPE_OPTIONS[] = "X-Content-Type-Options";
    const char HEADER_X_FORWARDED_HOST[] = "x-forwarded-host";
    const char HEADER_X_FRAME_OPTIONS[] = "X-Frame-Options";
    const char HEADER_X_XSS_PROTECTION[] = "X-XSS-Protection";

That definitely doesn't look good (for readability).

EDIT: I thought that when something is not included in the code guidelines, you should stick with what is already there, for consistency.

@Chocobo1
Copy link
Member Author

Chocobo1 commented Jun 2, 2017

yes

Alright, I'll open another PR for this.

but why you want to be different?

To save us another transition when some day we implements HTTP/2 (which all headers are in lowercase).

@sledgehammer999
Copy link
Member

I absolutely have no input in this. I don't want to get my hands dirty with webui code. If you want a cursory approval I can do it.
In the meantime let's not forget @OpenGG who initially reported the original problem.

@sledgehammer999 sledgehammer999 added the WebUI WebUI-related issues/changes label Jun 2, 2017
@sledgehammer999
Copy link
Member

Quick question: Does this PR warrant a new stable version aka v3.3.14?

@naikel
Copy link
Contributor

naikel commented Jun 2, 2017

@sledgehammer999 We would have to compile the changes and test them against severals third-party applications to be 100% sure it's stable

@sledgehammer999
Copy link
Member

@sledgehammer999 We would have to compile the changes and test them against severals third-party applications to be 100% sure it's stable

v3.3.13 was released because it defended against CSRF. Now this PR relaxes the conditions. So I am asking if it is important to be released as soon as possible(v3.3.14) or wait for v3.4.0...

@Taloth
Copy link

Taloth commented Jun 3, 2017

I'd say yes, it's worth 3.3.14, coz the longer version 3.3.13 is the latest, the more users will encounter problems.

@ngosang
Copy link
Member

ngosang commented Jun 5, 2017

@Chocobo1 I agree with @naikel , although HTTP headers are case insensitive, most web servers/browsers use capitalization.

@Chocobo1
Copy link
Member Author

Chocobo1 commented Jun 6, 2017

@ngosang
#6887 (comment)

@sledgehammer999
Copy link
Member

Seems that no one objects after ~11 days. So merging.
Thx for this.
New version with this will probably appear during the weekend.

@JourneyOver
Copy link

Any ETA on when this fix will be coming out?

@sledgehammer999
Copy link
Member

I'll try to release this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebUI WebUI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants