Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR - #151

Draft
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token
Draft

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR#151
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token

Conversation

@rootkiller6788

Copy link
Copy Markdown

Summary

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with an empty Token field as a connection error of type FRAME_ENCODING_ERROR.

quiche currently parses an empty token successfully and delivers it to the connection visitor, leaving the connection open (the token is later silently ignored by the TLS handshaker).

Change

In QuicFramer::ProcessIetfFrameData, after ProcessNewTokenFrame succeeds, reject an empty token with QUIC_INVALID_FRAME_DATA, which maps to FRAME_ENCODING_ERROR on the wire (see QuicErrorCodeToTransportErrorCode).

Tests

  • Added QuicFramerTest.NewTokenFrameEmptyToken, which verifies a NEW_TOKEN frame with a zero-length Token is rejected with QUIC_INVALID_FRAME_DATA.
  • Updated NewTokenFrameInstigateAcks, ServerClosesConnectionOnNewTokenFrame, and AckElicitingFrames to use a non-empty token, since an empty token is now correctly rejected before reaching QuicConnection::OnNewTokenFrame.

The server's own NEW_TOKEN send path (QuicSession::SendNewToken) always writes a non-empty token (it prepends a kAddressTokenPrefix byte and returns early if the address token is empty), so this change does not affect the legitimate send path.

Closes#135 (and the duplicate #133).

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to
be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with
an empty Token field as a connection error of type FRAME_ENCODING_ERROR.
quiche currently parses an empty token successfully and delivers it to
the connection, leaving the connection open. Reject the empty token in
the framer and raise QUIC_INVALID_FRAME_DATA, which maps to
FRAME_ENCODING_ERROR on the wire.
Update existing tests that used empty NEW_TOKEN frames as a convenience
so they exercise non-empty tokens, and add a framer test covering the
empty-token rejection.
@rootkiller6788

Copy link
Copy Markdown
Author

Hello, I've addressed the current merge state by syncing this branch with the latest upstream main. The branch had fallen behind main (the PR was based on 0a6b55f, while main has since advanced). I merged the current main into reject-empty-new-token and it came in cleanly with no conflicts. The PR diff is unchanged in substance: it still contains exactly the empty-token rejection in QuicFramer::ProcessIetfFrameData plus the corresponding test updates in quic_framer_test.cc and quic_connection_test.cc. For reference, the checks that run for external-contributor PRs — check-changes (GitHub Actions Scan) and cla/google — are all green. The remaining required status appears to be Google's internal presubmit, which I believe can only be triggered by a maintainer; if so, could you please take a look and run it when convenient? No behavior beyond the original fix has changed: a NEW_TOKEN frame with an empty Token field is now rejected with QUIC_INVALID_FRAME_DATA (mapped to FRAME_ENCODING_ERROR on the wire, per RFC 9000 §19.7), and the affected tests use non-empty tokens. Thank you for the review — please let me know if anything else needs adjustment.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty NEW_TOKEN Is Accepted Instead of FRAME_ENCODING_ERROR

1 participant

@rootkiller6788
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR - #151

Draft
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token
Draft

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR#151
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token

Conversation

@rootkiller6788

Copy link
Copy Markdown

Summary

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with an empty Token field as a connection error of type FRAME_ENCODING_ERROR.

quiche currently parses an empty token successfully and delivers it to the connection visitor, leaving the connection open (the token is later silently ignored by the TLS handshaker).

Change

In QuicFramer::ProcessIetfFrameData, after ProcessNewTokenFrame succeeds, reject an empty token with QUIC_INVALID_FRAME_DATA, which maps to FRAME_ENCODING_ERROR on the wire (see QuicErrorCodeToTransportErrorCode).

Tests

  • Added QuicFramerTest.NewTokenFrameEmptyToken, which verifies a NEW_TOKEN frame with a zero-length Token is rejected with QUIC_INVALID_FRAME_DATA.
  • Updated NewTokenFrameInstigateAcks, ServerClosesConnectionOnNewTokenFrame, and AckElicitingFrames to use a non-empty token, since an empty token is now correctly rejected before reaching QuicConnection::OnNewTokenFrame.

The server's own NEW_TOKEN send path (QuicSession::SendNewToken) always writes a non-empty token (it prepends a kAddressTokenPrefix byte and returns early if the address token is empty), so this change does not affect the legitimate send path.

Closes#135 (and the duplicate #133).

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to
be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with
an empty Token field as a connection error of type FRAME_ENCODING_ERROR.
quiche currently parses an empty token successfully and delivers it to
the connection, leaving the connection open. Reject the empty token in
the framer and raise QUIC_INVALID_FRAME_DATA, which maps to
FRAME_ENCODING_ERROR on the wire.
Update existing tests that used empty NEW_TOKEN frames as a convenience
so they exercise non-empty tokens, and add a framer test covering the
empty-token rejection.
@rootkiller6788

Copy link
Copy Markdown
Author

Hello, I've addressed the current merge state by syncing this branch with the latest upstream main. The branch had fallen behind main (the PR was based on 0a6b55f, while main has since advanced). I merged the current main into reject-empty-new-token and it came in cleanly with no conflicts. The PR diff is unchanged in substance: it still contains exactly the empty-token rejection in QuicFramer::ProcessIetfFrameData plus the corresponding test updates in quic_framer_test.cc and quic_connection_test.cc. For reference, the checks that run for external-contributor PRs — check-changes (GitHub Actions Scan) and cla/google — are all green. The remaining required status appears to be Google's internal presubmit, which I believe can only be triggered by a maintainer; if so, could you please take a look and run it when convenient? No behavior beyond the original fix has changed: a NEW_TOKEN frame with an empty Token field is now rejected with QUIC_INVALID_FRAME_DATA (mapped to FRAME_ENCODING_ERROR on the wire, per RFC 9000 §19.7), and the affected tests use non-empty tokens. Thank you for the review — please let me know if anything else needs adjustment.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty NEW_TOKEN Is Accepted Instead of FRAME_ENCODING_ERROR

1 participant

@rootkiller6788
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR - #151

Draft
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token
Draft

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR#151
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token

Conversation

@rootkiller6788

Copy link
Copy Markdown

Summary

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with an empty Token field as a connection error of type FRAME_ENCODING_ERROR.

quiche currently parses an empty token successfully and delivers it to the connection visitor, leaving the connection open (the token is later silently ignored by the TLS handshaker).

Change

In QuicFramer::ProcessIetfFrameData, after ProcessNewTokenFrame succeeds, reject an empty token with QUIC_INVALID_FRAME_DATA, which maps to FRAME_ENCODING_ERROR on the wire (see QuicErrorCodeToTransportErrorCode).

Tests

  • Added QuicFramerTest.NewTokenFrameEmptyToken, which verifies a NEW_TOKEN frame with a zero-length Token is rejected with QUIC_INVALID_FRAME_DATA.
  • Updated NewTokenFrameInstigateAcks, ServerClosesConnectionOnNewTokenFrame, and AckElicitingFrames to use a non-empty token, since an empty token is now correctly rejected before reaching QuicConnection::OnNewTokenFrame.

The server's own NEW_TOKEN send path (QuicSession::SendNewToken) always writes a non-empty token (it prepends a kAddressTokenPrefix byte and returns early if the address token is empty), so this change does not affect the legitimate send path.

Closes#135 (and the duplicate #133).

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to
be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with
an empty Token field as a connection error of type FRAME_ENCODING_ERROR.
quiche currently parses an empty token successfully and delivers it to
the connection, leaving the connection open. Reject the empty token in
the framer and raise QUIC_INVALID_FRAME_DATA, which maps to
FRAME_ENCODING_ERROR on the wire.
Update existing tests that used empty NEW_TOKEN frames as a convenience
so they exercise non-empty tokens, and add a framer test covering the
empty-token rejection.
@rootkiller6788

Copy link
Copy Markdown
Author

Hello, I've addressed the current merge state by syncing this branch with the latest upstream main. The branch had fallen behind main (the PR was based on 0a6b55f, while main has since advanced). I merged the current main into reject-empty-new-token and it came in cleanly with no conflicts. The PR diff is unchanged in substance: it still contains exactly the empty-token rejection in QuicFramer::ProcessIetfFrameData plus the corresponding test updates in quic_framer_test.cc and quic_connection_test.cc. For reference, the checks that run for external-contributor PRs — check-changes (GitHub Actions Scan) and cla/google — are all green. The remaining required status appears to be Google's internal presubmit, which I believe can only be triggered by a maintainer; if so, could you please take a look and run it when convenient? No behavior beyond the original fix has changed: a NEW_TOKEN frame with an empty Token field is now rejected with QUIC_INVALID_FRAME_DATA (mapped to FRAME_ENCODING_ERROR on the wire, per RFC 9000 §19.7), and the affected tests use non-empty tokens. Thank you for the review — please let me know if anything else needs adjustment.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty NEW_TOKEN Is Accepted Instead of FRAME_ENCODING_ERROR

1 participant

@rootkiller6788
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR - #151

Draft
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token
Draft

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR#151
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token

Conversation

@rootkiller6788

Copy link
Copy Markdown

Summary

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with an empty Token field as a connection error of type FRAME_ENCODING_ERROR.

quiche currently parses an empty token successfully and delivers it to the connection visitor, leaving the connection open (the token is later silently ignored by the TLS handshaker).

Change

In QuicFramer::ProcessIetfFrameData, after ProcessNewTokenFrame succeeds, reject an empty token with QUIC_INVALID_FRAME_DATA, which maps to FRAME_ENCODING_ERROR on the wire (see QuicErrorCodeToTransportErrorCode).

Tests

  • Added QuicFramerTest.NewTokenFrameEmptyToken, which verifies a NEW_TOKEN frame with a zero-length Token is rejected with QUIC_INVALID_FRAME_DATA.
  • Updated NewTokenFrameInstigateAcks, ServerClosesConnectionOnNewTokenFrame, and AckElicitingFrames to use a non-empty token, since an empty token is now correctly rejected before reaching QuicConnection::OnNewTokenFrame.

The server's own NEW_TOKEN send path (QuicSession::SendNewToken) always writes a non-empty token (it prepends a kAddressTokenPrefix byte and returns early if the address token is empty), so this change does not affect the legitimate send path.

Closes#135 (and the duplicate #133).

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to
be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with
an empty Token field as a connection error of type FRAME_ENCODING_ERROR.
quiche currently parses an empty token successfully and delivers it to
the connection, leaving the connection open. Reject the empty token in
the framer and raise QUIC_INVALID_FRAME_DATA, which maps to
FRAME_ENCODING_ERROR on the wire.
Update existing tests that used empty NEW_TOKEN frames as a convenience
so they exercise non-empty tokens, and add a framer test covering the
empty-token rejection.
@rootkiller6788

Copy link
Copy Markdown
Author

Hello, I've addressed the current merge state by syncing this branch with the latest upstream main. The branch had fallen behind main (the PR was based on 0a6b55f, while main has since advanced). I merged the current main into reject-empty-new-token and it came in cleanly with no conflicts. The PR diff is unchanged in substance: it still contains exactly the empty-token rejection in QuicFramer::ProcessIetfFrameData plus the corresponding test updates in quic_framer_test.cc and quic_connection_test.cc. For reference, the checks that run for external-contributor PRs — check-changes (GitHub Actions Scan) and cla/google — are all green. The remaining required status appears to be Google's internal presubmit, which I believe can only be triggered by a maintainer; if so, could you please take a look and run it when convenient? No behavior beyond the original fix has changed: a NEW_TOKEN frame with an empty Token field is now rejected with QUIC_INVALID_FRAME_DATA (mapped to FRAME_ENCODING_ERROR on the wire, per RFC 9000 §19.7), and the affected tests use non-empty tokens. Thank you for the review — please let me know if anything else needs adjustment.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty NEW_TOKEN Is Accepted Instead of FRAME_ENCODING_ERROR

1 participant

@rootkiller6788
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR - #151

Draft
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token
Draft

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR#151
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token

Conversation

@rootkiller6788

Copy link
Copy Markdown

Summary

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with an empty Token field as a connection error of type FRAME_ENCODING_ERROR.

quiche currently parses an empty token successfully and delivers it to the connection visitor, leaving the connection open (the token is later silently ignored by the TLS handshaker).

Change

In QuicFramer::ProcessIetfFrameData, after ProcessNewTokenFrame succeeds, reject an empty token with QUIC_INVALID_FRAME_DATA, which maps to FRAME_ENCODING_ERROR on the wire (see QuicErrorCodeToTransportErrorCode).

Tests

  • Added QuicFramerTest.NewTokenFrameEmptyToken, which verifies a NEW_TOKEN frame with a zero-length Token is rejected with QUIC_INVALID_FRAME_DATA.
  • Updated NewTokenFrameInstigateAcks, ServerClosesConnectionOnNewTokenFrame, and AckElicitingFrames to use a non-empty token, since an empty token is now correctly rejected before reaching QuicConnection::OnNewTokenFrame.

The server's own NEW_TOKEN send path (QuicSession::SendNewToken) always writes a non-empty token (it prepends a kAddressTokenPrefix byte and returns early if the address token is empty), so this change does not affect the legitimate send path.

Closes#135 (and the duplicate #133).

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to
be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with
an empty Token field as a connection error of type FRAME_ENCODING_ERROR.
quiche currently parses an empty token successfully and delivers it to
the connection, leaving the connection open. Reject the empty token in
the framer and raise QUIC_INVALID_FRAME_DATA, which maps to
FRAME_ENCODING_ERROR on the wire.
Update existing tests that used empty NEW_TOKEN frames as a convenience
so they exercise non-empty tokens, and add a framer test covering the
empty-token rejection.
@rootkiller6788

Copy link
Copy Markdown
Author

Hello, I've addressed the current merge state by syncing this branch with the latest upstream main. The branch had fallen behind main (the PR was based on 0a6b55f, while main has since advanced). I merged the current main into reject-empty-new-token and it came in cleanly with no conflicts. The PR diff is unchanged in substance: it still contains exactly the empty-token rejection in QuicFramer::ProcessIetfFrameData plus the corresponding test updates in quic_framer_test.cc and quic_connection_test.cc. For reference, the checks that run for external-contributor PRs — check-changes (GitHub Actions Scan) and cla/google — are all green. The remaining required status appears to be Google's internal presubmit, which I believe can only be triggered by a maintainer; if so, could you please take a look and run it when convenient? No behavior beyond the original fix has changed: a NEW_TOKEN frame with an empty Token field is now rejected with QUIC_INVALID_FRAME_DATA (mapped to FRAME_ENCODING_ERROR on the wire, per RFC 9000 §19.7), and the affected tests use non-empty tokens. Thank you for the review — please let me know if anything else needs adjustment.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty NEW_TOKEN Is Accepted Instead of FRAME_ENCODING_ERROR

1 participant

@rootkiller6788
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR - #151

Draft
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token
Draft

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR#151
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token

Conversation

@rootkiller6788

Copy link
Copy Markdown

Summary

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with an empty Token field as a connection error of type FRAME_ENCODING_ERROR.

quiche currently parses an empty token successfully and delivers it to the connection visitor, leaving the connection open (the token is later silently ignored by the TLS handshaker).

Change

In QuicFramer::ProcessIetfFrameData, after ProcessNewTokenFrame succeeds, reject an empty token with QUIC_INVALID_FRAME_DATA, which maps to FRAME_ENCODING_ERROR on the wire (see QuicErrorCodeToTransportErrorCode).

Tests

  • Added QuicFramerTest.NewTokenFrameEmptyToken, which verifies a NEW_TOKEN frame with a zero-length Token is rejected with QUIC_INVALID_FRAME_DATA.
  • Updated NewTokenFrameInstigateAcks, ServerClosesConnectionOnNewTokenFrame, and AckElicitingFrames to use a non-empty token, since an empty token is now correctly rejected before reaching QuicConnection::OnNewTokenFrame.

The server's own NEW_TOKEN send path (QuicSession::SendNewToken) always writes a non-empty token (it prepends a kAddressTokenPrefix byte and returns early if the address token is empty), so this change does not affect the legitimate send path.

Closes#135 (and the duplicate #133).

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to
be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with
an empty Token field as a connection error of type FRAME_ENCODING_ERROR.
quiche currently parses an empty token successfully and delivers it to
the connection, leaving the connection open. Reject the empty token in
the framer and raise QUIC_INVALID_FRAME_DATA, which maps to
FRAME_ENCODING_ERROR on the wire.
Update existing tests that used empty NEW_TOKEN frames as a convenience
so they exercise non-empty tokens, and add a framer test covering the
empty-token rejection.
@rootkiller6788

Copy link
Copy Markdown
Author

Hello, I've addressed the current merge state by syncing this branch with the latest upstream main. The branch had fallen behind main (the PR was based on 0a6b55f, while main has since advanced). I merged the current main into reject-empty-new-token and it came in cleanly with no conflicts. The PR diff is unchanged in substance: it still contains exactly the empty-token rejection in QuicFramer::ProcessIetfFrameData plus the corresponding test updates in quic_framer_test.cc and quic_connection_test.cc. For reference, the checks that run for external-contributor PRs — check-changes (GitHub Actions Scan) and cla/google — are all green. The remaining required status appears to be Google's internal presubmit, which I believe can only be triggered by a maintainer; if so, could you please take a look and run it when convenient? No behavior beyond the original fix has changed: a NEW_TOKEN frame with an empty Token field is now rejected with QUIC_INVALID_FRAME_DATA (mapped to FRAME_ENCODING_ERROR on the wire, per RFC 9000 §19.7), and the affected tests use non-empty tokens. Thank you for the review — please let me know if anything else needs adjustment.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty NEW_TOKEN Is Accepted Instead of FRAME_ENCODING_ERROR

1 participant

@rootkiller6788
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR - #151

Draft
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token
Draft

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR#151
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token

Conversation

@rootkiller6788

Copy link
Copy Markdown

Summary

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with an empty Token field as a connection error of type FRAME_ENCODING_ERROR.

quiche currently parses an empty token successfully and delivers it to the connection visitor, leaving the connection open (the token is later silently ignored by the TLS handshaker).

Change

In QuicFramer::ProcessIetfFrameData, after ProcessNewTokenFrame succeeds, reject an empty token with QUIC_INVALID_FRAME_DATA, which maps to FRAME_ENCODING_ERROR on the wire (see QuicErrorCodeToTransportErrorCode).

Tests

  • Added QuicFramerTest.NewTokenFrameEmptyToken, which verifies a NEW_TOKEN frame with a zero-length Token is rejected with QUIC_INVALID_FRAME_DATA.
  • Updated NewTokenFrameInstigateAcks, ServerClosesConnectionOnNewTokenFrame, and AckElicitingFrames to use a non-empty token, since an empty token is now correctly rejected before reaching QuicConnection::OnNewTokenFrame.

The server's own NEW_TOKEN send path (QuicSession::SendNewToken) always writes a non-empty token (it prepends a kAddressTokenPrefix byte and returns early if the address token is empty), so this change does not affect the legitimate send path.

Closes#135 (and the duplicate #133).

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to
be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with
an empty Token field as a connection error of type FRAME_ENCODING_ERROR.
quiche currently parses an empty token successfully and delivers it to
the connection, leaving the connection open. Reject the empty token in
the framer and raise QUIC_INVALID_FRAME_DATA, which maps to
FRAME_ENCODING_ERROR on the wire.
Update existing tests that used empty NEW_TOKEN frames as a convenience
so they exercise non-empty tokens, and add a framer test covering the
empty-token rejection.
@rootkiller6788

Copy link
Copy Markdown
Author

Hello, I've addressed the current merge state by syncing this branch with the latest upstream main. The branch had fallen behind main (the PR was based on 0a6b55f, while main has since advanced). I merged the current main into reject-empty-new-token and it came in cleanly with no conflicts. The PR diff is unchanged in substance: it still contains exactly the empty-token rejection in QuicFramer::ProcessIetfFrameData plus the corresponding test updates in quic_framer_test.cc and quic_connection_test.cc. For reference, the checks that run for external-contributor PRs — check-changes (GitHub Actions Scan) and cla/google — are all green. The remaining required status appears to be Google's internal presubmit, which I believe can only be triggered by a maintainer; if so, could you please take a look and run it when convenient? No behavior beyond the original fix has changed: a NEW_TOKEN frame with an empty Token field is now rejected with QUIC_INVALID_FRAME_DATA (mapped to FRAME_ENCODING_ERROR on the wire, per RFC 9000 §19.7), and the affected tests use non-empty tokens. Thank you for the review — please let me know if anything else needs adjustment.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty NEW_TOKEN Is Accepted Instead of FRAME_ENCODING_ERROR

1 participant

@rootkiller6788
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR - #151

Draft
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token
Draft

Reject empty NEW_TOKEN frames with FRAME_ENCODING_ERROR#151
rootkiller6788 wants to merge 2 commits into
google:mainfrom
rootkiller6788:reject-empty-new-token

Conversation

@rootkiller6788

Copy link
Copy Markdown

Summary

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with an empty Token field as a connection error of type FRAME_ENCODING_ERROR.

quiche currently parses an empty token successfully and delivers it to the connection visitor, leaving the connection open (the token is later silently ignored by the TLS handshaker).

Change

In QuicFramer::ProcessIetfFrameData, after ProcessNewTokenFrame succeeds, reject an empty token with QUIC_INVALID_FRAME_DATA, which maps to FRAME_ENCODING_ERROR on the wire (see QuicErrorCodeToTransportErrorCode).

Tests

  • Added QuicFramerTest.NewTokenFrameEmptyToken, which verifies a NEW_TOKEN frame with a zero-length Token is rejected with QUIC_INVALID_FRAME_DATA.
  • Updated NewTokenFrameInstigateAcks, ServerClosesConnectionOnNewTokenFrame, and AckElicitingFrames to use a non-empty token, since an empty token is now correctly rejected before reaching QuicConnection::OnNewTokenFrame.

The server's own NEW_TOKEN send path (QuicSession::SendNewToken) always writes a non-empty token (it prepends a kAddressTokenPrefix byte and returns early if the address token is empty), so this change does not affect the legitimate send path.

Closes#135 (and the duplicate #133).

RFC 9000 Section 19.7 requires the Token field in a NEW_TOKEN frame to
be non-empty, and a client MUST treat receipt of a NEW_TOKEN frame with
an empty Token field as a connection error of type FRAME_ENCODING_ERROR.
quiche currently parses an empty token successfully and delivers it to
the connection, leaving the connection open. Reject the empty token in
the framer and raise QUIC_INVALID_FRAME_DATA, which maps to
FRAME_ENCODING_ERROR on the wire.
Update existing tests that used empty NEW_TOKEN frames as a convenience
so they exercise non-empty tokens, and add a framer test covering the
empty-token rejection.
@rootkiller6788

Copy link
Copy Markdown
Author

Hello, I've addressed the current merge state by syncing this branch with the latest upstream main. The branch had fallen behind main (the PR was based on 0a6b55f, while main has since advanced). I merged the current main into reject-empty-new-token and it came in cleanly with no conflicts. The PR diff is unchanged in substance: it still contains exactly the empty-token rejection in QuicFramer::ProcessIetfFrameData plus the corresponding test updates in quic_framer_test.cc and quic_connection_test.cc. For reference, the checks that run for external-contributor PRs — check-changes (GitHub Actions Scan) and cla/google — are all green. The remaining required status appears to be Google's internal presubmit, which I believe can only be triggered by a maintainer; if so, could you please take a look and run it when convenient? No behavior beyond the original fix has changed: a NEW_TOKEN frame with an empty Token field is now rejected with QUIC_INVALID_FRAME_DATA (mapped to FRAME_ENCODING_ERROR on the wire, per RFC 9000 §19.7), and the affected tests use non-empty tokens. Thank you for the review — please let me know if anything else needs adjustment.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty NEW_TOKEN Is Accepted Instead of FRAME_ENCODING_ERROR

1 participant

@rootkiller6788