-
Notifications
You must be signed in to change notification settings - Fork 459
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 for challenge where part of formula is stored in a div #206
Closed
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
1d39b35
Fix for challenge where part of formula is stored in a div
lukele fe79caa
Fix syntax error.
lukele 2d22225
Consider query string when building redirect URL
lukele f4e9f9a
Change user agent on each instantiation.
lukele 814519c
Use cloudflares delay by default before submitting the challenge answer.
lukele 3c8fa45
Make sure the headers are sent in proper order.
lukele e5c2292
Define the new g variable.
lukele 6538fb8
Avoid removing too much.
lukele File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right here you are overwriting the existing headers that are present. For example, after sticking a debugger in here above this line.
self.headers
already exists and has contents.For my request to be successful, I need the
Content-Type
header to remain. Other than that - the commit works well, hopefully we can get this in soon.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, could you explain this change? It also changes some other headers that are needed, I also assume that, for the servers that respect it, we don't want to limit them to English. The 'Accept' also may varry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CodyWoolaver These headers seemed to be working best with Cloudflare, but I agree that breaking support for custom headers doesn't make sense. Can you share a site where you have problems with my current version.
The following gist contains a version which tries to restore support for custom headers by only using best working headers as long as cloudflare has not been bypassed:
https://gist.github.com/lukele/ce188004545192c0d92064e85138f0ab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you could set those headers for specifically interacting with cloudflare and preserve the interactions with the intended site. That or you could merge in the specific headers you have picked and that way the intended headers could still make it through.
The site that this caused issues for was actually a local install of deluge and their webapi. There is a hard requirement to have Content-Type when interacting with it (no idea why). The client accessing it is https://github.com/SickChill/SickChill/tree/master/lib/cfscrape and uses a copy of cfscrape.
Let me know if I can help in any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's basically what I'm doing in the gist. The only problem is that Cloudflare did require for example the user-agent not to change between requests, so merging might be more problematic. Did you try the gist by any chance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense - to preserve the User-Agent as it is a variable that should not frequently change. Is there any risk with picking a couple of headers and making them stay that way?
For example force the header values of
User-Agent Connection Upgrade-Insecure-Requests
to be consistent and dokwargs['headers'].update(<contents>)
and that way the user agent, and other important headers remain and we can still pass in custom headers.I have not had a chance to test it out yet, I won't be in a position to do so for several hours. I can get back to you then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also like to mention, that I completely removed lines 67-79 of this PR in the code running at home and was able to get through without a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any more movement on this @lukele? The proposed fix you provided did work, however I still believe we should not be completely ignoring the headers if they are passed in. Having a set of headers that we require to go to cloudflare makes sense, but it should be an update on the object, not a replacement of.
@Anorov do you have any input on this? I know some people are looking for a more prominent fix, rather than trying to cherry-pick this pr :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CodyWoolaver sorry for the late reply. Best have a look at cloudscraper (https://github.com/VeNoMouS/cloudscraper). It‘s based on cfscrape but more actively maintained for now. It also includes some good fixes regarding the sometimes presented captcha challenge.