api/pkg/authconfig: reject multiple JSON documents in Decode#51919
api/pkg/authconfig: reject multiple JSON documents in Decode#51919vvoland merged 1 commit intomoby:masterfrom
Conversation
| func decode(r io.Reader) (*registry.AuthConfig, error) { | ||
| authConfig := ®istry.AuthConfig{} | ||
| if err := json.NewDecoder(r).Decode(authConfig); err != nil { | ||
| decoder := json.NewDecoder(r) | ||
| if err := decoder.Decode(authConfig); err != nil { | ||
| // always return an (empty) AuthConfig to increase compatibility with | ||
| // the existing API. | ||
| return ®istry.AuthConfig{}, invalid(fmt.Errorf("invalid JSON: %w", err)) | ||
| } | ||
|
|
||
| // Reject multiple JSON documents to prevent potential security issues | ||
| // where additional data could be silently ignored. | ||
| var extra json.RawMessage | ||
| if err := decoder.Decode(&extra); err == nil { | ||
| // Additional JSON document found - this should not be allowed | ||
| return ®istry.AuthConfig{}, invalid(errors.New("multiple JSON documents not allowed")) | ||
| } else if !errors.Is(err, io.EOF) { | ||
| // Unexpected trailing data (not valid JSON, not EOF) | ||
| return ®istry.AuthConfig{}, invalid(fmt.Errorf("unexpected trailing data: %w", err)) | ||
| } | ||
|
|
||
| return authConfig, nil | ||
| } |
There was a problem hiding this comment.
This can be greatly simplified; decoder already has an option for this to check if more content is present;
func decode(r io.Reader) (*registry.AuthConfig, error) {
authConfig := ®istry.AuthConfig{}
dec := json.NewDecoder(r)
if err := dec.Decode(authConfig); err != nil {
// always return an (empty) AuthConfig to increase compatibility with
// the existing API.
return ®istry.AuthConfig{}, invalid(fmt.Errorf("invalid JSON: %w", err))
}
if dec.More() {
return ®istry.AuthConfig{}, invalid(errors.New("multiple JSON documents not allowed"))
}
return authConfig, nil
}There was a problem hiding this comment.
Thank you for the excellent suggestion! Using Decoder.More() makes the code much simpler and more efficient.
I've updated the implementation as you suggested
a06d84e
All tests pass with the simplified implementation.
vvoland
added
area/api
|
Changes look good; we should squash the commits. Also I don't think there's a real security issue here; the code expects a single auth-header value, so is expected to use the first value; additional values can safely be ignore, but it's good to notify the user that the value is incorrect. |
|
Did a quick rebase and squash, but looks like GitHub actions is unhappy right now (issue with Windows runners?) |
Explicitly reject multiple JSON documents in authentication payloads instead of silently ignoring them. This helps notify users when the authentication payload is incorrectly formatted. Signed-off-by: hiroto.toyoda <[email protected]>
Sorry for the conflict! I was working on squashing the commits at the same time you were doing the rebase. My force push may have overwritten your changes. Should I pull your version, or is the current state acceptable? Happy to defer to your rebased version if you prefer. |
|
GitHub actions has some issues with their runners; I'll try restarting CI later (no need to push again to start it) |
|
Changes look good; I think the only diff would be the commit-message, so no problem! |
3 participants
This fixes an issue where multiple JSON documents in the authentication payload would be silently ignored. The decoder now explicitly rejects any trailing data after the first valid JSON document.
- What I did
Fixed an issue in the
authconfigpackage where multiple JSON documents in authentication payloads were silently ignored.The
decode()function now explicitly validates that only a single JSON document is present and rejects any trailing data.- How I did it
decode()function to perform an additionalDecode()call after successfully parsing the first JSON documentio.EOF(expected) or finds additional data (error)json.RawMessageinstead ofanyfor efficient existence checking without full parsing of potentially large trailing data- How to verify it
Run the test suite:
All tests pass, including the new test cases that verify multiple JSON documents are properly rejected.
Benchmark results show minimal performance impact:
go test -bench=BenchmarkDecodeAuthConfig -benchmem- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)
