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

ws: Reject frames with unknown reserved bits set #16069

Closed
wants to merge 4 commits into from

Conversation

ADKaster
Copy link
Contributor

RFC 6455 Section 5.2 notes that for bits RSV1, RSV2, and RSV3 of the framing header, a non-zero value that is not defined by a negotiated extension MUST Fail the WebSocket connection.

Related to #16065, which will assign meaning to RSV1 if the permessage-deflate extension was negotiated on the connection.

RFC 6455 Section 5.2 notes that for bits RSV1, RSV2, and RSV3
of the framing header, a non-zero value that is not defined
by a negotiated extension MUST Fail the WebSocket connection.
@github-actions github-actions bot added the tests label Jan 21, 2025
lib/ws.c Outdated Show resolved Hide resolved
tests/libtest/lib2310.c Outdated Show resolved Hide resolved
@testclutch
Copy link

Analysis of PR #16069 at 2a60872e:

Test 3028 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Generated by Testclutch

@ADKaster
Copy link
Contributor Author

My new test failed in a mingw job, but I see that some WebSocket tests are disabled for that config:

https://github.com/curl/curl/actions/runs/12903536775/job/35978986490?pr=16069#step:14:4233

# FIXME: WebSockets test results ignored due to frequent failures on native Windows:

Doesn't feel right to add it to the ignore list myself though 😅

@vszakats
Copy link
Member

vszakats commented Jan 22, 2025

It's a flaky job, unrelated to this patch.

It'd be nice to drop WebSockets from the ignore list on Windows, but some of those tests are flaky still on this platform, according to latest test runs.

@vszakats
Copy link
Member

vszakats commented Jan 22, 2025

Update: this job fails the same after restart, and may be related:

FAIL 2310: 'WebSockets unknown reserved bit set in frame header' WebSockets

Edit: fails consistently on the 3rd run.

@ADKaster
Copy link
Contributor Author

I copied most of the test file from 2302, which seems to be skipped on that configuration. Should I look into this, skip the test, or wait for a more windows-y contributor to take a look?

@vszakats
Copy link
Member

vszakats commented Jan 22, 2025

I copied most of the test file from 2302, which seems to be skipped on that configuration. Should I look into this, skip the test, or wait for a more windows-y contributor to take a look?

If you'd like to look into it, this would be a good opportunity. I'm not sure what it takes.

If too much trouble, I think we should add 2310 to all the places in windows.yml where 2302 is already made an exception.

lib/ws.c Outdated Show resolved Hide resolved
lib/ws.c Outdated Show resolved Hide resolved
@github-actions github-actions bot added the CI Continuous Integration label Jan 27, 2025
@ADKaster
Copy link
Contributor Author

If you'd like to look into it, this would be a good opportunity. I'm not sure what it takes.

I don't have a mingw environment handy, and my application doesn't plan to support windows for several years, so I think I'm going to have to punt on investigating curl websocket issues in that environment.

@bagder bagder closed this in 1b740ae Jan 28, 2025
@bagder
Copy link
Member

bagder commented Jan 28, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration tests WebSocket
Development

Successfully merging this pull request may close these issues.

4 participants