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

New Feature: set 'credentials=include' to support additional CI systems #157

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

Conversation

marvin-enthus
Copy link

Hi,

we found your great project while we were searching for a good visualization for the trivy artefacts.

As we are using Jenkins for our CI Pipelines we need a different authentication mechanism. After some investigation we decided the most efficient way would be to use the existing authentication the user already has. This can be archived by setting the adequate CORS headers on the CI server - but it also requires an option on the fetch request.

With this PR we want to contribute the changes we made back to this project. The option on the fetch request could be enabled by setting a checkbox in the "Authorization" config page. Additionally the option could be enabled by adding a 'includeCredentials=true' query parameter to the url.

Bye,
Chris

@Morl99
Copy link
Contributor

Morl99 commented Dec 29, 2021

I am trying to grasp this change. Am I right, that by setting credentials to include, fetch will include the session credentials of the target domain in the form of cookies that match the target domain? If that is the case, is it really necessary to make this configurable? I think this sounds like a sensible default for the trivy-vulnerability-explorer, since it requires a correctly configured CORS anyways. Or would it prevent the usage if CORS is not configured accordingly, even if the cookies are not actually needed?

If that is the case, I would opt for only making this configurable, and not exposing it via the URL, for the sake of simplicity. Setting up the integration between the trivy-vulnerability-explorer is a one time thing since the settings are persisted in the local storage. It might be even a better idea to set credentials to include if (and only if) there is no special auth header. Or do you need the flag (and the cookies) as well as a special header?

@marvin-enthus
Copy link
Author

Hi,

during our debugging we have seen the logon redirect in the request debugger when credentials: include was set and the CORS header were not configured correctly.

I did some investigation on the specs / documentation to credentials: include. My current findings:
In https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#sending_a_request_with_credentials_included it has a note on CORS-Wildcard definitions but also mentions https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#requests_with_credentials for CORS details.
And on this page it states:

Since this [(must be the CORS request in this context)] is a simple GET request, it is not preflighted but the browser will reject any response that does not have the Access-Control-Allow-Credentials: true header, and not make the response available to the invoking web content.

For me it reads like: even if we have theen a response in the (browser-) debugger this should not be forwarded to the application.

Having this in mind it doesn't seem to be a good idea to make this a sensible default.

I would be fine with only making this configurable. Maybe it would be an Idea to maintain a 'configured' flag in LocalStorage and pop up the configuration when there is no configuration on the client itself? To show the user there is no current configuration.

Bye,
Chris

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.

2 participants