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

Use selected fields from app in context to avoid secrets #4350

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jamstah
Copy link
Collaborator

@Jamstah Jamstah commented May 14, 2024

The app struct includes sensitive data that the handlers don't necessarily need access to, so remove it from the context, and don't pass the app in place of contexts.

Where handlers do need configuration data from the app, store it in the handler's own struct instead of relying on it being in context.

The app struct includes sensitive data that the handlers don't
necessarily need access to, so remove it from the context, and don't
pass the app in place of contexts.

Where handlers do need configuration data from the app, store it in the
handler's own struct instead of relying on it being in context.

Signed-off-by: James Hewitt <james.hewitt@uk.ibm.com>
@Jamstah
Copy link
Collaborator Author

Jamstah commented May 14, 2024

This "fixes" some false positives from CodeQL discussed here: github/codeql#16486

There's one thing I'm unsure about - the repo remover currently in handlers.Context is an instance of a distribution.Namespace that supports the remover interface. As its the distribution.Namespace instance that contains the potential secrets, this PR removes it from the context.

The repo remover that we stored in handlers.Context never gets used in the codebase, but it is exported, so that may be considered a breaking change to the API... but I'm not super familiar with how that works for consumers.

@Jamstah
Copy link
Collaborator Author

Jamstah commented May 14, 2024

An alternative to this PR is to just mark the false positives as such, but I'd already done this work as part of investigating, and I believe that cleaning up our contexts is something we're interested in doing anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant