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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Whitelist buttons on query log pages #1578

Merged
merged 1 commit into from
Nov 8, 2020

Conversation

sgtlaggy
Copy link
Contributor

@sgtlaggy sgtlaggy commented Sep 5, 2020

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:
Fix the behavior of the Whitelist button on both the short-term and long-term query log pages.
Previously, domains blocked due to cases

  • 5 (exact blacklist),
  • 9 (gravity),
  • 10 (regex blacklist CNAME), and
  • 11 (exact blacklist CNAME)

would blacklist the domain instead of whitelisting it when clicking the Whitelist button.

How does this PR accomplish the above?:
It corrects a typo of data[5] by using array.indexOf instead of several === checks. If === would be preferable, I can change that upon request.
It also corrects the omission of 3 whitelist buttons from the click event, which erroneously blacklisted a domain when clicking the "Whitelist" button.

What documentation changes (if any) are needed to support this PR?:
None

Signed-off-by: sgtlaggy <8661717+sgtlaggy@users.noreply.github.com>
@sgtlaggy
Copy link
Contributor Author

sgtlaggy commented Sep 5, 2020

Neglected to mention in commit (for auto close), this should close #1348 and maybe #1533.

@PromoFaux
Copy link
Member

I seem unable to reproduce the problem that this PR is designed to fix...

I have tried on both current master and current devel, and in both cases pressing whitelist correctly adds to the whitelist

I might need @DL6ER's eyes on this one please...

Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR specifically fixes the whitelisting button for the recently added deep CNAME inspection blocking types. In addition, it fixes checking the wrong field for exactly blacklisted domains.

@DL6ER
Copy link
Member

DL6ER commented Nov 8, 2020

@sgtlaggy I edited your description slightly as status 5 is exact black-, not whitelist. From your PR it is clear that this was merely a typo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants