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

middleware for content type and accept headers #14075

Merged
merged 8 commits into from
Jun 4, 2024

Conversation

james-prysm
Copy link
Contributor

@james-prysm james-prysm commented Jun 3, 2024

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Adding middleware for handling content type and accept header checks.
Only impacts the beacon apis for now https://ethereum.github.io/beacon-APIs

  • future needs to handle keymanager apis and validator apis

Which issues(s) does this PR fix?

Fixes #14054

Other notes for review

@james-prysm james-prysm changed the title middleware for content type middleware for content type and accept headers Jun 3, 2024
@james-prysm james-prysm added the API Api related tasks label Jun 3, 2024
@james-prysm james-prysm marked this pull request as ready for review June 3, 2024 22:00
@james-prysm james-prysm requested a review from a team as a code owner June 3, 2024 22:00
@james-prysm james-prysm requested review from nalepae, saolyn and potuz June 3, 2024 22:00
@james-prysm james-prysm added the Ready For Review A pull request ready for code review label Jun 3, 2024
@james-prysm james-prysm requested review from rkapka and removed request for potuz June 3, 2024 22:01
Comment on lines 80 to 93
accepted := false
acceptTypes := strings.Split(acceptHeader, ",")
for _, acceptType := range acceptTypes {
acceptType = strings.TrimSpace(acceptType)
for _, serverAcceptedType := range serverAcceptedTypes {
if strings.HasPrefix(acceptType, serverAcceptedType) {
accepted = true
break
}
}
if accepted {
break
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should follow https://datatracker.ietf.org/doc/html/rfc2616#section-14.1. I don't think we have to support all edge cases, but */* and type "/" "*" would be nice to have.

beacon-chain/rpc/endpoints.go Outdated Show resolved Hide resolved
beacon-chain/rpc/endpoints.go Outdated Show resolved Hide resolved
beacon-chain/rpc/endpoints.go Outdated Show resolved Hide resolved
beacon-chain/rpc/endpoints.go Outdated Show resolved Hide resolved
},
{
template: "/eth/v1/beacon/pool/attester_slashings",
name: namespace + ".SubmitAttesterSlashing",
handler: server.SubmitAttesterSlashing,
methods: []string{http.MethodPost},
middleware: []mux.MiddlewareFunc{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing accept header

},
{
template: "/eth/v1/beacon/states/{state_id}/validators",
name: namespace + ".GetValidators",
handler: server.GetValidators,
methods: []string{http.MethodGet, http.MethodPost},
middleware: []mux.MiddlewareFunc{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing accept header

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

james-prysm and others added 5 commits June 4, 2024 13:17
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
@james-prysm james-prysm added this pull request to the merge queue Jun 4, 2024
Merged via the queue into develop with commit 1b40f94 Jun 4, 2024
16 of 17 checks passed
@james-prysm james-prysm deleted the content-type-middleware branch June 4, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check conten-type and return 415 if not supported by route
2 participants