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

fix: fixes for handling more 3xx codes and not asking for trust twice #1771

Merged
merged 3 commits into from
Mar 31, 2024

Conversation

quintesse
Copy link
Contributor

No description provided.

We were handling only a limited set of redirect types, this has now been
extended to include all common and not so common 3xx redirect codes.
This includes the somewhat strange 303 See Other where the spec isn't
completely clear but the general concensus seems to be that it forces
the next request to be a GET request.

Fixes jbangdev#1769
@@ -193,6 +204,24 @@ protected String getJSon(Collection<String> rules) {
return trustedsources;
}

public void addTemporary(String trust) {
Util.infoMsg("Adding temporary " + trust);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this shows up weird..how is this different from "trust once" option ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the "trust once" option, thing is that the "trust once" was literally that, it would only trust once. So if you have some code that tries to access the same URL twice, for example when retrying, then you're actually accessing the same URL multiple times... which would result in the trust question to be asked multiple times for the same URL!

This is because there was no "memory" for the "trust once" option. So with this change we will keep track of all the URLs that the user answered "1" to for the duration of the JBang execution, they will be forgotten once JBang exits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok - then let's use the same terminology. Something like:

"Trusting for this one run: URL" or similar.

Or we rename the question to be Trusting temporarily - or Trussting for run or something similar.

Just to reduce confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do 👍

Copy link
Collaborator

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

the "adding temporary url" message is not great - need to find better way for that to be clearer what is being added temporarily and for how long.

@quintesse
Copy link
Contributor Author

@maxandersen how about these new messages?

@maxandersen
Copy link
Collaborator

Good ones!

@maxandersen maxandersen merged commit eceb5c1 into jbangdev:main Mar 31, 2024
12 checks passed
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.

None yet

2 participants