Stop cookies from duplicating on each item processed #12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As opposed to appending individual cookies each time which causes duplicate cookies to be added to requests until they are too long to send. Note that there was some logic initially for excluding certain cookies. This doesn't seem to be required for EBI to work, requests work correctly either way. The removed snippet was added when attempting to support self-signed SSL and I have tested that self-signed SSL still works after this change.
edalex-ian
reviewed
Nov 9, 2022
When you start importing it runs a login again and grabs the token at the start so that isn't a problem, the problem persists over individual items but not over multiple import runs. So it works like:
|
|
Hmmm, yeah that was the scenario I was concerned about. But perhaps oEQ is returning the session cookie with every response and so the problem is avoided. |
edalex-ian
approved these changes
Nov 10, 2022
There was a problem hiding this comment.
@SammyIsConfused has confirmed the session cookie is returned in every response, so this should be good to go.
|
Looks like you'll probably want to build/do a release for this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The purpose of this PR is to change how cookies are grabbed from server responses and appended to future requests.
So previously these would be appended to the cookie jar on each iteration, which causes a duplicate of each cookie per item imported until the requests are too long to be sendable as the cookie jar is maintained between each item processed.
My fix for this involves simply grabbing the set-cookie value from the header and using that as the contents of the cookie jar, replacing the contents of the jar rather than appending to it.
Note that the way this previously was supposed to work was to only append cookies that were not "PATH", "DOMAIN", "EXPIRES", "SECURE", or "HTTPONLY" cookies. Not only did this not seem to be working (these cookies would still appear in the cookie jar and get duplicated) but these cookies do not seem to prevent EBI from working at all. If it turns out that this is required I can work to retain this logic but after a chat with @nelson-edalex and some testing it doesn't appear to be al that necessary.
The snippet that this PR replaces was added in this commit when adding support for self-signed certificates, and so I have tested with an openEQUELLA using self-signed certificates as well as real certificates and can confirm that it still works both ways.
This bug was found when testing openEQUELLA using AWS Load Balancer, which adds two cookies AWSALB and AWSALBCORS to the header. These were quite long and so after duplicating 10 times or so the cookies would be too long to send a request, limiting the amount of items that can be processed at once to just 10. I have tested against AWS LB with over a hundred items and confirmed that this no longer happens.
It would be beneficial to get @nelson-edalex to test this too as this tool is used heavily in consulting and the change was as a result of a bug found by him.