-
Notifications
You must be signed in to change notification settings - Fork 53
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 authenticity_token bug #192
Conversation
@@ -86,6 +86,7 @@ | |||
visit '/projects/new' | |||
fill_in 'project_description', with: '_italic_ **bold**' | |||
click_link 'Preview' | |||
wait_for_preview |
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.
Hm... have_css
already waits the Capybara.default_wait_time
so would page.find('#preview-contents')
. Why do we need this method? What are you trying to achieve?
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.
here capybara doesn't wait for our ajax request of previewing markdown to complete so it doesn't find that div.
So from this link, I tried to add this function but the error still persists sometimes 😕 .
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.
Did you follow the first link from your blog post? This is what the capybara README has to say:
When working with asynchronous JavaScript, you might come across situations
where you are attempting to interact with an element which is not yet present
on the page. Capybara automatically deals with this by waiting for elements to
appear on the page.
The rest of the thoughtbot blog post is about fixing race conditions between two requests happening at the same time (click_link
which triggers the ajax request & reload_page
), this is not what is happening here.
So effectively you just have increased the time capbybara waits for the bold
and italic
elements to appear from 2 seconds (default) to 7 seconds. Which you can also do by setting Capybara.default_wait_time = 7
instead of adding all this code.
So back to my original question, why where you doing this? Did you run into a timeout locally? :-)
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.
Yes, I was having timeout error, that's why I tried to wait for ajax and also i dont want to increase capybara timeout error just for one feature testing.
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 a lot of code for changing something that probably depends on the hardware the test runs on. I would rather read Capybara.default_wait_time
from an environment variable so you can set this individually and we can leave the default as it is. Please do it this 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.
actually, I think instead of changing capybara's default wait time we should use wait as :
expect(page).to have_selector(:css, "em", text: 'italic', wait: 7)
because capybara will wait for every test whether some element does exist or not.
(please do tell me if I m wrong somewhere 😅 )
However, I think there's some other problem since none of these works, even after waiting or setting the time to 30 seconds! , that's why I tried all those lines of code but even those seem to pass only sometimes ...😕
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.
Try just increasing the Capybara.default_wait_time
in the helper, see if that helps.
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.
already did that, no success.
@@ -5,6 +5,7 @@ class ApplicationController < ActionController::Base | |||
before_filter :authenticate_user! | |||
before_filter :load_news | |||
before_filter :set_episode | |||
protect_from_forgery with: :null_session, if: Proc.new { |c| c.request.format == 'application/json' } |
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 fail to understand how this would fix the problem. It would just skip protection for POST
ing json, which we don't do, or do I miss something?
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.
The token provided by the rails in the beginning and the time we click submit are different because when ajax calls the markdown controller new auth token is provided so I skipped protection only if request format is of application/json.
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.
The token provided by the rails in the beginning and the time we click submit are different because when ajax calls the markdown controller new auth token is provided
Sure thing, I understand the problem :-)
so I skipped protection only if request format is of application/json.
What I don't understand is the solution.
- We GET
/markdown/preview?source='## blah'
as json.protect_from_forgery
isn't used for GET, hence your change doesn't apply. - Then we POST the same data to
/projects
as html. Yourprotect_from_forgery
before_filter
isn't used, as it's not json, hence your change doesn't apply.
How does that fix the 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.
my apologies for the late reply, I used
protect_from_forgery with: :null_session, if: Proc.new { |c| c.request.format == 'application/json' }
after checking these links : 1 & 2 ,since i don't want to close csrf check.
protect_from_forgery isn't used for GET
^ I didn't get any strong proof but I think maybe in line:8 protect_from_forgery is triggered only when request.format is application/json 🤔
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.
Skipping forgery protection in general is not something we want to do right. But as I said, the only type of requests where forgery protection is running are POST requests. I really don't think you can disable the token update for the session like this by skipping this.
Can you try something? When rendering the preview also change the token of the input field?
<input name="authenticity_token" value="$TOKEN" type="hidden">
There is a view helper called form_authenticity_token
that gives you the current token.
hey @AnkushMalik thank you for your contributions 💐😍 |
updates the auth token in the form resolve csrf error Closes SUSE#191
cbbd204
to
ba46151
Compare
failing test cases were resolved in this. |
Hey @AnkushMalik sorry that I was not responsive for so long. Too much stuff happening in |
Closes #191