Navigation Menu

Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[馃悰] Hacking instructor: DevTools open detection is CPU and RAM heavy and never stops #1525

Closed
cnotin opened this issue Dec 1, 2020 · 13 comments
Labels

Comments

@cnotin
Copy link
Contributor

cnotin commented Dec 1, 2020

馃悰 Bug report

Description

When doing a challenge with Hacking Instructor which checks for the opening of the devtools (e.g. "view basket" challenge) I noticed that the devtools console was going crazy in a loop as my CPU 馃く
image

I understand that this comes from the code that waits for devtools:
https://github.com/bkimminich/juice-shop/blob/d29573a5d88209ecbe482b2b2140ded6f17d6b5b/frontend/src/hacking-instructor/helpers/helpers.ts#L128-L131
And this potential issue was mentioned: https://stackoverflow.com/questions/7798748/find-out-whether-chrome-console-is-open/48287643#comment110980766_48287643 馃

Moreover, this never stops even after completing the challenge! We have to refresh the page to stop the loop.

Is this a regression?

No

馃敩 Minimal Reproduction

  1. Go to scoreboard
  2. Launch "view basket" challenge
  3. Follow instructions until you are asked to open devtools
  4. Open the devtools and go to the console tab
  5. Notice the many "" elements being logged, which is normal as it's the way this hack works... (which is a nice trick anyway! 馃槄)
  6. Follow the remaining instructions, clear the console, see how it continues to fill up

馃敟 Exception or Error

N/A

馃尦 Your Environment

N/A
Running latest code from develop branch as of now

Additional Information

Some suggestions:

  • Sleep longer between each check. Currently: 100ms but I think 500ms or even 1s might be reactive enough while reducing the load
  • At least stop it when the condition (devtools opened) is reached. I don't have enough mastery of async to suggest something sorry...
@cnotin cnotin added the bug label Dec 1, 2020
@J12934
Copy link
Member

J12934 commented Dec 2, 2020

Does this properly detect if your dev tools are opened?
The check should鈩笍 stop once the dev tools have been opened :)

Edit: Tried it out, wow you're totally right, had to force quit my browser as it wasn't responding anymore 馃槵馃槄

Might be possible if I understand the detection method correctly to use a "more" light weight element then the image element for the logs. Light weight meaning a element with less properties

@bkimminich
Copy link
Member

If the DevTools detection is only possible in a "whacky" way, then we can also drop it entirely as it was before. The tutorial would still be fine, it just made it feel more smooth when waiting for the DevTool to open. But it's by no means essential.

@J12934
Copy link
Member

J12934 commented Dec 2, 2020

https://github.com/sindresorhus/devtools-detect

Seems pretty stable and easy to use

@cnotin
Copy link
Contributor Author

cnotin commented Dec 2, 2020

The current solution works fine and detects when I open the devtools but I confirm that it continues in the background (but I cannot say why because of my limited understanding of its implementation with a promise...)
It's a very nice thing to have (and I was actually very curious to see how it detected it!) so it would be nice to keep it! I think it could work good enough with a higher sleep time and by making sure it properly stops the loop

I've also found the devtools-detect that you mentioned @J12934, but I find its implementation to be more unreliable because it relies on checking the dimension of the display:

		const widthThreshold = window.outerWidth - window.innerWidth > threshold;
		const heightThreshold = window.outerHeight - window.innerHeight > threshold;

It also doesn't work if the devtools are opened in a second window...

@J12934
Copy link
Member

J12934 commented Dec 3, 2020

Mh took another look,
For me, it's working correctly in chrome, meaning it properly detects open devtools and stops the logging immediately after the detection.
In Firefox it doesn't work at all, I have to click the Hacking Instructor to continue there. This also causes firefox to be really badly impacted by this check as it never stops.

It possible to really improve the performance costs by running console.clear() behind the console.dir call in the check. This ensures that the console never gets overwhelmed by printing all of those entires, as it always only prints the last one.

@cnotin
Copy link
Contributor Author

cnotin commented Dec 3, 2020

I re-checked and indeed I confused Firefox and Chrome (was using both at the moment...). And I agree with you @J12934:

For me, it's working correctly in chrome, meaning it properly detects open devtools and stops the logging immediately after the detection.
In Firefox it doesn't work at all, I have to click the Hacking Instructor to continue there. This also causes firefox to be really badly impacted by this check as it never stops.

I tried to add console.clear as you said and indeed it still does not work on Firefox, but it does not make it suffer too much anymore (CPU and memory are fine, but the loop continues though), and the detection still work properly on Chrome.

@stale
Copy link

stale bot commented Dec 19, 2020

This issue has been automatically marked as stale because it has not had recent activity. 馃搯 It will be closed automatically in one week if no further activity occurs.

@stale stale bot added the stale label Dec 19, 2020
@bkimminich
Copy link
Member

@J12934 @cnotin Do you guys think something needs to be done here still?

@stale stale bot removed the stale label Dec 21, 2020
@J12934
Copy link
Member

J12934 commented Dec 21, 2020

Did we update this?
Don't remember seeing / doing any changes to the actual code, aside from experimentations.

@bkimminich
Copy link
Member

Yeah, me neither, but I wasn't entirely sure if your conclusion was to change anything or just leave it as it is... 馃榿

@J12934
Copy link
Member

J12934 commented Dec 21, 2020

I think the co conclusion was that we can add a console.clear call to fix the performance problems on both chrome and Firefox, but the detection will still only work in chrome.

Only downside would be that this would clear all console history of the current tab. This should not really be an issue in my view.

@cnotin
Copy link
Contributor Author

cnotin commented Dec 21, 2020

Same conclusion here :)
Let's try with this, which is still a progress!

@github-actions
Copy link

This thread has been automatically locked because it has not had recent activity after it was closed. 馃敀 Please open a new issue for regressions or related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants