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

Source code added for toolforge tool_srish(wikimedia) #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nikitiwari
Copy link
Owner

@tgr
Please review the code. Its the wikimedia_toolforge tool for displaying the recent wikipedia edits.

index.html Outdated
<head>
<title>index</title>
<meta charset="utf-8"/>
<link href="https://fonts.googleapis.com/css?family=Roboto" rel="stylesheet">
Copy link

Choose a reason for hiding this comment

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

We try to avoid using third-party resources as that means sending personally identifiable information (IP addresses, at least) to those servers. You can probably use https://tools.wmflabs.org/fontcdn/ instead; if not, you can always self-host. (Same goes for external JS libraries vs. https://tools.wmflabs.org/cdnjs/ )

Copy link
Owner Author

Choose a reason for hiding this comment

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

url of Roboto font stylesheet and jquery script has been updated.

index.js Outdated
event.preventDefault();
$("#output").empty();
var username = searchTerm.val();
var url = 'https://en.wikipedia.org/w/api.php?origin=*&action=query&format=json&list=usercontribs&ucuser={{searchTerm}}&ucdir=newer&uclimit=5&callback=?'.replace('{{searchTerm}}', username);
Copy link

Choose a reason for hiding this comment

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

If you want to use template-like notation (which is nice) but don't want to pull in a templating library just because of that (which is reasonable), you could write a simple helper function to do the replacement.

Whether you do that or not, make sure to add proper escaping. Usernames can't contain HTML but they can contain special characters like & or ? (and it's good practice to assume they could contain anything, anyway).

Copy link

Choose a reason for hiding this comment

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

Also, there isn't much point in using both origin=* (which is for CORS) and callback=? (which forces the request to be JSONP).

Copy link
Owner Author

Choose a reason for hiding this comment

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

can you please give some examples on proper escaping about what data has to be prevented?

callback=? has been removed , further origin=* is to test the project locally.

Copy link

Choose a reason for hiding this comment

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

Let's say the user is called A&B, then the URL would be + var url = 'https://en.wikipedia.org/w/api.php?origin=*&action=query&format=json&list=usercontribs&ucuser=A&B&ucdir=newer&uclimit=5. A webserver will interpret that like

origin  => *
action  => query
format  => json
list    => usercontribs
ucuser  => A
B       =>
ucdir   => newer
uclimit => 5
 

which refers to a different user.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@tgr sir,
I have updated my js file and resolved this issue using encodeURI function .Please review it.
Due to some technical issue I could not upload my file on time , sorry for the delay.

@nikitiwari nikitiwari self-assigned this Oct 22, 2017
@tgr
Copy link

tgr commented Oct 23, 2017

The escaping happens too late, you should only escape the username. I don't think it will work like this. Also, use encodeURIComponent; the goal is to encode special characters like & and encodeURI doesn't do that.

Looks good otherwise; note though that putting the script on Toolforge is part of the task.

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.

2 participants