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 country_code is null #1975

Merged
merged 1 commit into from
Aug 3, 2020
Merged

Fix country_code is null #1975

merged 1 commit into from
Aug 3, 2020

Conversation

klezVirus
Copy link
Contributor

Pull Request

Hi, noticed there is a little issue that makes zombies disappear in the console.
#1712
The issue has been closed, but other people are still facing it.

Category

Bug in the Admin interface

Feature/Issue Description

The issue is simple, BeeF doesn't show hooked browser, but it's still logging them, as observable by the image below:
image

From the web console, it's easy to see that the issue is presenting at the presentation layer (it's a JavaScript file: web_ui_all.js):
image

Technical Background

The code in web_ui_all.js is minified, and the error triggers in the following snippet (notice the same error appears twice in the same file):

((c += " <img width='13px' height='13px' class='zombie-tree-icon' src='/ui/media/images/icons/country-squared/" + escape(e.country_code.toLowerCase()) + ".svg' /> "),                      

The value e.country_code is not null checked, hence causing the error. As we_ui_all.js is a minified file generated on access, modyfing it it's not a solution. The real file affected is zombiesTreeList.js.

The fix is trivial, adding a null check to the existing "Unknown" check:

if ( !hooked_browser.country || !hooked_browser.country_code || hooked_browser.country == 'Unknown' ) {
	balloon_text += " <img width='13px' height='13px' class='zombie-tree-icon' src='<%= @base_path %>/media/images/icons/unknown.png' /> ";
	balloon_text += "Location: Unknown";
} else {
	balloon_text += " <img width='13px' height='13px' class='zombie-tree-icon' src='<%= @base_path %>/media/images/icons/country-squared/" + escape(hooked_browser.country_code.toLowerCase()) + ".svg' /> ";
	balloon_text += "Location: " + hooked_browser.city + ", " + hooked_browser.country;
}

...

if ( !hooked_browser.country || !hooked_browser.country_code || hooked_browser.country == 'Unknown' ) {
	text += "<img width='13px' height='13px' class='zombie-tree-icon' src='<%= @base_path %>/media/images/icons/unknown.png' /> ";
} else {
	text += "<img width='13px' height='13px' class='zombie-tree-icon' src='<%= @base_path %>/media/images/icons/country-squared/" + escape(hooked_browser.country_code.toLowerCase()) + ".svg' /> ";
}

Test Cases

I've tested it arbitrarily setting null values for hooked browsers. They were still appearing in the Admin Console.

@bcoles
Copy link
Collaborator

bcoles commented Aug 1, 2020

Resolves #1750
Resolves #1712

This is a good start. The underlying issue still needs to be resolved as per #1750 (comment). That is, the reason why null is returned needs to be investigated.

Copy link
Collaborator

@bcoles bcoles left a comment

Choose a reason for hiding this comment

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

Untested but LGTM

@bcoles bcoles requested a review from jcrew99 August 1, 2020 08:55
@jcrew99
Copy link
Collaborator

jcrew99 commented Aug 3, 2020

I haven't been able to replicate the error so its hard to say to the fix itself.
However testing with the change doesn't cause any issues for manual testing and automated testing so its good in my mind.

Its adding null checking which is always a good idea anyway, since you are happy as well @bcoles ill merge :) .
Well done @klezVirus on your fix

@jcrew99 jcrew99 merged commit 8876f69 into beefproject:master Aug 3, 2020
Copy link
Collaborator

@jcrew99 jcrew99 left a comment

Choose a reason for hiding this comment

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

Manual testing and automated testing passes

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