src: make Sec-WebSocket-Key check case-insensitive #7248

MylesBorins · 2016-06-09T16:33:27Z

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
the commit message follows commit guidelines

Affected core subsystem(s)

src

Description of change

Current case sensitive comparison is breaking netty-based WS clients.

replace strncmp with strncasecmp

Fixes: #7247

MylesBorins · 2016-06-09T16:34:23Z

/cc @nodejs/diagnostics @ofrobots @develar

addaleax · 2016-06-09T16:34:36Z

src/inspector_socket.cc

@@ -445,7 +445,7 @@ static int header_value_cb(http_parser* parser, const char* at, size_t length) {
  struct http_parsing_state_s* state = (struct http_parsing_state_s*)
      (reinterpret_cast<inspector_socket_t*>(parser->data))->http_parsing_state;
  state->parsing_value = true;
-  if (state->current_header && strncmp(state->current_header,
+  if (state->current_header && strncasecmp(state->current_header,
                                       SEC_WEBSOCKET_KEY_HEADER,
                                       sizeof(SEC_WEBSOCKET_KEY_HEADER)) == 0) {


You may want to keep the arguments aligned here :-)

fixed. Needing to put things on a new line as the sizeof line was spilling over 80 characters

Sorry to be so nit-picky, but ideally the subsequent arguments of strncasecmp should start at the same column at which the first one does (basically like they were before)

got it this time I think :P

cjihrig · 2016-06-09T16:38:01Z

LGTM pending @addaleax's comment.

bnoordhuis · 2016-06-09T17:01:44Z

src/inspector_socket.cc

+  if (state->current_header &&
+      strncasecmp(state->current_header,
+      SEC_WEBSOCKET_KEY_HEADER,
+      sizeof(SEC_WEBSOCKET_KEY_HEADER)) == 0) {


Can you add a version of StringEqualNoCase() that takes a size parameter and use that?

Style: arguments should line up here.

lining up the arguments pushes over 80 characters... what is the best way to deal with that?

edit: nvm got the line up... looking into StringEqualNoCase

pavelfeldman · 2016-06-09T19:20:18Z

lgtm

MylesBorins · 2016-06-10T19:44:08Z

updated based on @bnoordhuis' suggestions. PTAL

ofrobots · 2016-06-10T20:46:19Z

@thealphanerd now that you have added StringEqualNoCaseN it would be good to add some tests in test/cctest/util.cc.

MylesBorins · 2016-06-10T22:34:00Z

@ofrobots updated with test. Let me know if you would like some more cases
ci: https://ci.nodejs.org/job/node-test-pull-request/2981/

ofrobots · 2016-06-10T23:52:46Z

src/util-inl.h

+  for (int i = 0; i < length; i++) {
+    if (ToLower(a[i]) != ToLower(b[i]))
+      return false;
+    if (a[i] == '\0')


I think we are guaranteed at this point that if a[i] is null, then b[i] is also null. I think you can simply return true if a[i] is null. The second if below can simply be dropped.

It would be good to add a test that compares: "abc\0abc" and "abc\0xyz". Your function should (already does) return true for this. This matches the semantics of strncmp.

ofrobots · 2016-06-10T23:54:50Z

LGTM w/ comment.

MylesBorins · 2016-06-11T00:17:43Z

Updated to address @ofrobots comments. PTAL

edit:

ci: https://ci.nodejs.org/job/node-test-pull-request/2985/

bnoordhuis · 2016-06-11T10:17:05Z

src/inspector_socket.cc

@@ -1,4 +1,5 @@
 #include "inspector_socket.h"
+#include "util-inl.h"


#include "util.h" first.

MylesBorins · 2016-06-11T16:47:37Z

Updated based on @bnoordhuis's suggestions PTAL

ci: https://ci.nodejs.org/job/node-test-pull-request/2990/

bnoordhuis · 2016-06-11T17:38:58Z

src/util-inl.h

@@ -219,6 +219,16 @@ bool StringEqualNoCase(const char* a, const char* b) {
  return false;
 }

+bool StringEqualNoCaseN(const char* a, const char* b, size_t length) {
+  for (int i = 0; i < length; i++) {


bnoordhuis · 2016-06-11T17:41:37Z

LGTM with comment.

MylesBorins · 2016-06-11T19:46:44Z

updated PTAL

MylesBorins · 2016-06-13T18:06:53Z

If there are no other concerns with this PR I will merge it tomorrow morning

Current case sensitive comparison is breaking netty-based WS clients. replace strncmp with strncasecmp Fixes: nodejs#7247 PR-URL: nodejs#7248 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Current case sensitive comparison is breaking netty-based WS clients. replace strncmp with strncasecmp Fixes: #7247 PR-URL: #7248 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Jun 9, 2016

MylesBorins mentioned this pull request Jun 9, 2016

v8_inspector: any netty-based WS client cannot connect to inspector because Sec-WebSocket-Key check is not case-insensitive #7247

Closed

addaleax reviewed Jun 9, 2016
View reviewed changes

MylesBorins changed the title ~~src: Make Sec-WebSocket-Key check case-insensitive~~ src: make Sec-WebSocket-Key check case-insensitive Jun 9, 2016

MylesBorins force-pushed the inspect-ws-case-insensitive branch from 9d97d5b to f3baec9 Compare June 9, 2016 16:35

MylesBorins force-pushed the inspect-ws-case-insensitive branch from f3baec9 to 9611926 Compare June 9, 2016 16:38

bnoordhuis reviewed Jun 9, 2016
View reviewed changes

MylesBorins force-pushed the inspect-ws-case-insensitive branch from 9611926 to 8ee040e Compare June 9, 2016 17:04

develar mentioned this pull request Jun 10, 2016

Add v8_inspector support #6792

Closed

MylesBorins force-pushed the inspect-ws-case-insensitive branch from 8ee040e to ffb8530 Compare June 10, 2016 19:43

MylesBorins force-pushed the inspect-ws-case-insensitive branch from ffb8530 to f7269c7 Compare June 10, 2016 22:33

MylesBorins force-pushed the inspect-ws-case-insensitive branch from f7269c7 to 071889c Compare June 10, 2016 22:35

ofrobots reviewed Jun 10, 2016
View reviewed changes

MylesBorins force-pushed the inspect-ws-case-insensitive branch from 071889c to 493e0ce Compare June 11, 2016 00:17

MylesBorins force-pushed the inspect-ws-case-insensitive branch from 493e0ce to 263d9ef Compare June 11, 2016 00:19

bnoordhuis reviewed Jun 11, 2016
View reviewed changes

src/inspector_socket.cc

@@ -1,4 +1,5 @@

#include "inspector_socket.h"

#include "util-inl.h"

Copy link

Member

bnoordhuis Jun 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include "util.h" first.

MylesBorins force-pushed the inspect-ws-case-insensitive branch from 263d9ef to ec6604a Compare June 11, 2016 16:47

bnoordhuis reviewed Jun 11, 2016
View reviewed changes

MylesBorins force-pushed the inspect-ws-case-insensitive branch from ec6604a to f5c8dc6 Compare June 11, 2016 19:46

MylesBorins force-pushed the inspect-ws-case-insensitive branch from f5c8dc6 to 02fd124 Compare June 13, 2016 18:07

MylesBorins force-pushed the inspect-ws-case-insensitive branch from 02fd124 to f1d1071 Compare June 14, 2016 18:10

MylesBorins merged commit f1d1071 into nodejs:master Jun 14, 2016

MylesBorins added this to the 7.0.0 milestone Jun 14, 2016

MylesBorins mentioned this pull request Jun 14, 2016

Track backport of v8_inspector nodejs/Release#109

Closed

MylesBorins added the dont-land-on-v6.x label Jun 14, 2016

Fishrock123 removed the dont-land-on-v6.x label Jul 5, 2016

Fishrock123 mentioned this pull request Jul 5, 2016

Propose v6.3.0 (v2) #7550

Merged

MylesBorins deleted the inspect-ws-case-insensitive branch July 8, 2016 14:53

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the dont-land-on-v4.x label Jul 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: make Sec-WebSocket-Key check case-insensitive #7248

src: make Sec-WebSocket-Key check case-insensitive #7248

MylesBorins commented Jun 9, 2016

MylesBorins commented Jun 9, 2016 •

edited

Loading

addaleax Jun 9, 2016

MylesBorins Jun 9, 2016

addaleax Jun 9, 2016

MylesBorins Jun 9, 2016

cjihrig commented Jun 9, 2016

bnoordhuis Jun 9, 2016

MylesBorins Jun 9, 2016 •

edited

Loading

pavelfeldman commented Jun 9, 2016

MylesBorins commented Jun 10, 2016

ofrobots commented Jun 10, 2016

MylesBorins commented Jun 10, 2016 •

edited

Loading

ofrobots Jun 10, 2016

ofrobots commented Jun 10, 2016

MylesBorins commented Jun 11, 2016 •

edited

Loading

bnoordhuis Jun 11, 2016

MylesBorins commented Jun 11, 2016 •

edited

Loading

bnoordhuis Jun 11, 2016

bnoordhuis commented Jun 11, 2016

MylesBorins commented Jun 11, 2016 •

edited

Loading

MylesBorins commented Jun 13, 2016

		@@ -1,4 +1,5 @@
		#include "inspector_socket.h"
		#include "util-inl.h"

src: make Sec-WebSocket-Key check case-insensitive #7248

src: make Sec-WebSocket-Key check case-insensitive #7248

Conversation

MylesBorins commented Jun 9, 2016

Checklist

Affected core subsystem(s)

Description of change

MylesBorins commented Jun 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig commented Jun 9, 2016

Choose a reason for hiding this comment

MylesBorins Jun 9, 2016 • edited Loading

Choose a reason for hiding this comment

pavelfeldman commented Jun 9, 2016

MylesBorins commented Jun 10, 2016

ofrobots commented Jun 10, 2016

MylesBorins commented Jun 10, 2016 • edited Loading

Choose a reason for hiding this comment

ofrobots commented Jun 10, 2016

MylesBorins commented Jun 11, 2016 • edited Loading

Choose a reason for hiding this comment

MylesBorins commented Jun 11, 2016 • edited Loading

Choose a reason for hiding this comment

bnoordhuis commented Jun 11, 2016

MylesBorins commented Jun 11, 2016 • edited Loading

MylesBorins commented Jun 13, 2016

MylesBorins commented Jun 9, 2016 •

edited

Loading

MylesBorins Jun 9, 2016 •

edited

Loading

MylesBorins commented Jun 10, 2016 •

edited

Loading

MylesBorins commented Jun 11, 2016 •

edited

Loading

MylesBorins commented Jun 11, 2016 •

edited

Loading

MylesBorins commented Jun 11, 2016 •

edited

Loading