Repository navigation
Replies: 2 comments
TriageVerdict: a reproducible bug in how Is this a misuse of humus?No. The strongest case for misuse is that the walkthrough wraps the API in a handler that copies the request. But humus asks for that wrapping. #760, revision 1 says the API has "no size limit of the package's own: behind The strip case has no end user code in it at all. An end user can't defend against either with anything humus gives them. Trusting every caller would keep the strip from copying, but that turns failure injection's protection off. Is it a feature request?No. It contradicts three promises:
A fix restores these promises. It changes no exported name, signature, Is it reproducible?Yes. I ran each test 4 times under
The existing The walkthrough: the add is
A stripped scenario: the span is named
This corrects one claim of the report. It says whether the strip "names a span wrongly hasn't been checked". It does, with or without Whyotelhttp names the span from its own copy of the request, after the handler returns. At v0.72.0 it makes the copy at A ServeMux sets the pattern on the request it was given, at
In the walkthrough, both effects stack. The cart strips the scenario of a caller its #1295 is filed to fix it:
|
|
Fixed by #1295 in #1296, merged as d98170c. The openapi API now records the route it finds in the request's context, which every copy of the request shares. A new layer right below otelhttp in |
Uh oh!
There was an error while loading. Please reload this page.
What happens
In the API walkthrough,
example/shoppingcart, the server spans of both services are named after the method and/, not after the operation's route. An add isPOST /on the cart, and the catalog's answer isGET /.ServeHTTPinopenapi/v3says the spans are named after the route, such asGET /carts/{cartId}, andhttpserver's doc says the same when the handler setsRequest.Pattern.This was found while writing the walkthrough's observing page in #1120 (PR #1240). The page documents the spans as the code names them, and its Tempo query finds an add by
span.url.pathinstead of by its name.What's known
This hasn't been triaged. What follows comes from reading the code on main at 992e51b:
http.MaxBytesHandler, under aServeMux. The cart doesmux.Handle("/", http.MaxBytesHandler(api, maxBodyBytes))(app.go#L138), and the catalog does the same (app.go#L126).MaxBytesHandlerhands the API a shallow copy of the request. TheServeMuxhas already setPatternto/on the request otelhttp passed down. The API sets the route it found on the copy (api.go#L224-L232), and otelhttp, which renames the span from its own request after the handler returns, sees/.httpservermay lose the pattern too.stripScenariospassesr.WithContext(ctx)on when it strips an untrusted scenario (strip.go#L39-L46), so a pattern set below it would land on that copy. Whether that names a span wrongly hasn't been checked.observing.mdlistsPOST /andGET /and queries byspan.url.path.Not yet known: whether it reproduces in a test, whether the fix belongs in the walkthrough, in
openapi/v3or inhttpserver, and whether other walkthroughs that serve HTTP are affected.Expected
A request the API serves is in a server span named after its method and the operation's route, such as
POST /carts/{cartId}/itemsandGET /products/{productId}, asopenapi/v3andhttpserverdocument. The observing page names the spans that way.References
(*API).ServeHTTPinopenapi/v3/api.gohttpserver/service.go#L68-L80andhttpserver/doc.go#L121-L131All reactions