-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Feature/improve autocomplete js #18552
base: develop
Are you sure you want to change the base?
Conversation
… username input and login button
…tton CSS selectors
result = []; | ||
let buttons = document.querySelectorAll('button', parentElement); | ||
for (let button of buttons) { | ||
if (button.innerText.toLocaleLowerCase().includes('log')) { |
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 usually dislike code that is tied to a specific language. log
might work for login
and logga
(which I believe is login in .se
) but... for sites developed in other languages (spanish, polish, etc.) this will not work.
Also, sign-in
will not be found.
Can we find an alternative?
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 only thing that comes into my mind is to return all buttons located inside parentNode
. Hopefully there will be login button.
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 code is really good! 💯
Just minor changes and comments.
…_js.has_active_session if chrome belongs to outer scope
… so we return all buttons in parent node
@Q-back is this ready for a second review? |
@andresriancho Thanks for activity 😉 . There's one new improvement which I'd like to finish before review. |
…er exception handling during js login process
…entedChrome.focus() method
@andresriancho PR is ready for second review |
return result['result']['result']['value'] | ||
runtime_exception = result.get('result', {}).get('exceptionDetails') | ||
if runtime_exception: | ||
raise ChromeScriptRuntimeException( |
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.
This is new! Are you sure it is handled in all calls to this method? In the past we were returning None
and now we raise an exception. Make sure to search all calls for this method and modify error handling appropiately.
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.
Yeah, I have added few more fixes in this PR.
This works for sure. It's additional (new) exception which is raised when underlying script throws internal JS exception. We still return None
in other cases here https://github.com/andresriancho/w3af/pull/18552/files/8d520bc067455089a9fccac9c395f9114f521e8a#diff-4a954d4e5c6123fad237c66f5399b9dfR617
dom_analyzer.js
)has_active_session
method using the same chrome instance as_do_login()
method to preserve already logged in instanceusername_field_css_selector
login_button_css_selector
login_form_activator_css_selector