-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
📝 [Proposal]: Improve RFC Compliance #3383
Comments
We should tackle these each as a separate PR. Any that doesn't apply, we can |
@gaby Any screenshots of the |
@JIeJaitt It looks the same, the difference is that it takes longer to answer. For example that response above took almost 5 minutes. |
I believe q value negotiation is already implmented. Might want to check the accuracy of the rest of the findings. |
@nickajacks1 I forgot to feed some files to the model. Re-running the prompt now with the full-code base. |
@nickajacks1 The content negotiation is not fully RFC compliant. This is what Accept Header Parsing and Sorting
|
There is a way to parse media type params with the standard lib ( https://pkg.go.dev/mime#ParseMediaType ) but I didn't use it because there was a desire to minimize impact to performance. For media type params and content negotiation, the rfc does not detail how the params should be used. I modeled the behavior based on how it is done in express. If there are specific examples of breakage during quoted value parsing, it would be helpful to list them out. |
Feature Proposal Description
These are issues found by scanning
ctx.go
usingOpenAI - o1 pro
model for RFC compliance.1. Content Negotiation (Accept, Accept-Charset, etc.)
No q-weighting: The c.Accepts(...), c.AcceptsCharsets(...), c.AcceptsEncodings(...), and c.AcceptsLanguages(...) helpers do not interpret q= weighting parameters. The HTTP spec (RFC 9110 §12.5.1) indicates that clients can specify priority (weights) like Accept: text/html;q=0.8, application/json;q=1.0. Right now, these helpers simply select the first match if present, ignoring numeric q factors. This is a common “simplification,” but strictly is not RFC-compliant.
Ignoring the standard Forwarded header (RFC 7239): While not specifically about content negotiation, it’s another type of request header that is often relevant in proxy scenarios. The code currently only checks for X-Forwarded-Proto, X-Url-Scheme, X-Forwarded-Protocol, and X-Forwarded-Ssl. The standard Forwarded header (RFC 7239) is never parsed. This often results in incomplete or non-standard proxy handling.
2. Content-Encoding and Decompression
Ambiguity around deflate: The code claims to handle deflate by calling fasthttp.Request.BodyInflate(). However, the HTTP spec is famously ambiguous about deflate: strictly, “deflate” means zlib wrapper per RFC 9110 §8.4.1.2 (and historically RFC 2616). Some libraries interpret “deflate” as raw deflate with no zlib wrapper, which leads to interoperability issues. It is unclear from this snippet whether your “deflate” code is “zlib-based” or “raw.” This mismatch can cause non-compliant behavior with certain clients.
No mention of compress: The older compress content-encoding (LZW) is rarely used but still in the RFC. You do not support it. That is not necessarily a practical problem, but is a slight departure from the official list in RFC 9110.
Error handling returns the error text as the response body: In Body() → tryDecodeBodyInOrder(), if decompression fails, you do return []byte(err.Error()). An HTTP server would usually respond with an error status (400 Bad Request or 415 Unsupported Media Type) rather than returning the raw error string as the payload. The standard says that a request with invalid or partial compression is a client error.
3. Range Requests
Partial coverage of Range: The c.Range(size int) method parses a Range header into offsets but does not appear to handle the full RFC logic for “If-Range,” “multipart/byteranges” responses, or generating the correct Content-Range/206 Partial Content responses automatically. This is only partial coverage.
Also, returning the raw error ErrRangeMalformed or ErrRangeUnsatisfiable without actually generating an HTTP status code (e.g. 416 Range Not Satisfiable) is not strictly in line with RFC 9110 §14.4 (“If a valid Range header is not provided ... the server SHOULD return 416” or handle it consistently).
4. HEAD Method Semantics
No visible HEAD handling in SendFile, Send, etc.: RFC 9110 §9.3 requires that when the request is HEAD, the server must not send a message body, only headers—including the correct Content-Length if known. In plain Fiber usage, often the router or framework might handle HEAD automatically. However, from the snippet alone, it’s not obvious whether:
We skip writing bodies for HEAD requests, or We rely on fasthttp to do so, or We simply treat HEAD like GET. If HEAD is never specially handled, you can inadvertently send a response body to HEAD requests, which is incorrect per the RFC.
5. Ignoring Standard Expect: 100-continue
6. “Forwarded” and Other Proxy Headers
Non-RFC standard “Forwarded” header: Already mentioned above, but specifically, the code only inspects X-Forwarded-* and X-Url-Scheme. The modern standard is Forwarded: for=..., proto=..., host=... etc. This omission is common in many frameworks but is, strictly, incomplete handling.
Multiple Host header or invalid Host: The code does not appear to guard against multiple or invalid Host headers. Per RFC 9110, a request with multiple Host headers is invalid (400 Bad Request). That is not enforced here.
7. Vary Header Logic
c.Vary(...) and c.Append(...): While you do support appending fields to Vary, you do not verify whether those fields are already present (the code tries to skip duplicates, but it does a naive check strings.Contains(...); that is typically enough, but can lead to corner cases with partial matches.
No logic ensures that if you do content negotiation or send a different representation based on Accept-, that you must set Vary: Accept-.... This is best practice but the user must manually call c.Vary(...).
In short, it is mostly okay but could drift from the official guidance that a server “MUST add a Vary header” if the representation was selected based on certain request headers.
8. Case Sensitivity vs. RFC 9110
9. Handling of “deflate,” “gzip,” “chunked” in requests
Chunked request bodies: Because the code (and fasthttp) handle bodies in a certain non-blocking manner, you effectively do not see raw chunked data as the RFC describes. fasthttp does partial reading internally. So an advanced chunk extension from the client or trailing headers might not be handled in a fully standard way. This is more a known fasthttp quirk, but it is relevant from an RFC standpoint.
No fallback to identity: If the client sets something like Content-Encoding: compress or an unknown token, you basically skip tryDecodeBodyInOrder(...) and simply do return c.fasthttp.Request.Body(). The specification typically says if an unknown or unsupported Content-Encoding is used, that’s 415 Unsupported Media Type. In practice, many servers do pass the body through “as is,” so it is arguable whether that’s a violation or permissible.
10. Date, Server, and Other Standard Headers
The HTTP spec (RFC 9110 §7.1.1.2) indicates that the server should send a Date header in most responses. Many frameworks let the underlying HTTP library do it. fasthttp does not automatically set a Date header unless you configure it. That may be fine in practice, but strictly, the RFC states that an HTTP/1.1 server SHOULD send a Date if feasible.
Similarly, “Server” is often omitted for security reasons, but the RFC says that “Server” is recommended.
11. Edge Cases with SendFile and Ranged Requests
Range responses require Accept-Ranges: bytes if partial requests are supported.** In your SendFile(...) logic, if ByteRange is true, you do a partial read. The correct approach is to set Accept-Ranges: bytes so that clients know partial content is available. Some of this is handled by fasthttp.FS but watch for compliance.
HEAD requests for large files: Typically require returning the correct Content-Length but no body. The snippet does not show that logic explicitly—likely fasthttp implements it.
The text was updated successfully, but these errors were encountered: