Skip to content

Conversation

alnr
Copy link
Contributor

@alnr alnr commented Sep 8, 2025

Stumbled across this by chance.

@alnr alnr requested review from hperl and zepatrik September 8, 2025 15:11
@alnr alnr self-assigned this Sep 8, 2025
@alnr alnr requested review from aeneasr and a team as code owners September 8, 2025 15:11
Copy link

@gaultier gaultier left a comment

Choose a reason for hiding this comment

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

LGTM but there's something wrong with the CI setup

return errorsx.WithStack(ErrInvalidRequestURI.WithHintf("Unable to fetch OpenID Connect request parameters from 'request_uri' because: %s.", err.Error()).WithWrap(err).WithDebug(err.Error()))
}
defer response.Body.Close()
response.Body = io.NopCloser(io.LimitReader(response.Body, 10*1024*1024)) // limit to 10MiB
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the limit be configurable? You could make it a config property in Fosite struct itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's necessary. This is a defence mechanism only. Do you think 10MiB is too small?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hard to say, particularly with RAR. I don't see any reason why it shouldn't be parameterized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any reason why it shouldn't be parameterized.

The reason is that it's more complicated (more code).

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should be controlled by the OP using Fosite. Perhaps just a property in the struct? It doesn't have to be a full blown Configurator...

Sign up for free to 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.

3 participants