fix: Prevent infinitely recursing when injecting into iframes #66
Conversation
This patch prevents `axe-webdriverjs` to infinitely recurse when injecting `axe-core` into `<iframe>`s. I've re-written the code we use for injecting which will only recurse as deep as the number of nested `<iframe>`s on the page. In order to keep the code "easy to read/write", I've used `async/await` rather than chaining `Promise`s together. Because we do not have a clear picture of what Node.js versions we need to support here, I've added Babel in order to ensure `async/await` works in older Nodes. It is currently setup use support Node v4. A test case has been added ensuring this solves the problem described in #63. Closes #63.
| @@ -3,7 +3,7 @@ | |||
| "description": "Provides a method to inject and analyze web pages using aXe", | |||
| "version": "2.0.0", | |||
| "license": "MPL-2.0", | |||
| "main": "lib/index.js", | |||
| "main": "dist/index.js", | |||
There was a problem hiding this comment.
Don't forget to add the "engine" field
|
|
||
| this.didLogError = true; | ||
| // eslint-disable-next-line no-console | ||
| console.log('Failed to inject axe-core into one of the iframes!'); |
There was a problem hiding this comment.
Slight scope creep here, but I'd like for us to have the option to turn this off: #41
Axe-core 3.0 has a rule which checks that iframes have been tested. That's a better way to expose this info than this console log.
There was a problem hiding this comment.
Can that be taken care of outside of this PR? I'd like to get this in to fix the issue for a client.
| @@ -0,0 +1,82 @@ | |||
| class AxeInjector { | |||
There was a problem hiding this comment.
Why did you rewrite this to work as a class? As far as I can tell, all this is doing is binding some arguments to the inject function.
There was a problem hiding this comment.
I rewrote this as a class to more easily maintain state. If it weren't a class, we'd have several nested closures, which can get hairy quickly.
There was a problem hiding this comment.
I don't see why, but lets roll with it.
test/integration/gh-63.js
Outdated
| .manage() | ||
| .timeouts() | ||
| .setScriptTimeout(5000); | ||
| return driver.get('https://buy.garmin.com/en-US/US/p/591046'); |
There was a problem hiding this comment.
This seems more than a little fragile. Can we do something to reproduce the problem somewhere we control ourselves instead?
There was a problem hiding this comment.
I have not successfully reproduced this outside of the Garmin page. I think we'd have to create a Facebook "like button" and serve some HTML containing it over HTTPS, which seems like a lot of work.
Instead, should I put this test behind a flag (so it's conditionally run)?
We shouldn't rely on the state of 3rd-party endpoints in our tests. Additionally, the client may not want us hitting their URLs this way.
This patch prevents
axe-webdriverjsfrom infinitely recursing when injectingaxe-coreinto<iframe>s. I've re-written the code we use for injecting which will only recurse as deep as the number of nested<iframe>s on the page.In order to keep the code "easy to read/write", I've used
async/awaitrather than chainingPromises together. Because we do not have a clear picture of what Node.js versions we need to support here, I've added Babel in order to ensureasync/awaitworks in older Nodes. It is currently setup use support Node v4.A test case has been added ensuring this solves the problem described in #63.Closes #63.