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

Closes #17; Better Domain Filtering #18

Merged
merged 4 commits into from
Apr 24, 2015
Merged

Conversation

rsandor
Copy link
Contributor

@rsandor rsandor commented Apr 22, 2015

Use a regex to ensure that the domain filter appears at the end of the domain instead of anywhere.

@rsandor
Copy link
Contributor Author

rsandor commented Apr 22, 2015

@tjmehta - Please review.

@bkendall
Copy link
Contributor

it looks like your branch has duplicates of commits that are now on master... maybe rebase?

@rsandor
Copy link
Contributor Author

rsandor commented Apr 22, 2015

Just needed to merge them in. Sorry about that.

return names.filter(function (name) {
return ~name.indexOf(process.env.DOMAIN_FILTER);
return name.match(regexFilter);
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like you're filtering and allowing anything on a domain (good), but then looking for 0-infinite whitespace after before the end of the line? why not just trim?

];
query.resolve('127.0.0.1', names, function (err, records) {
if (err) { return done(err); }
expect(records).to.be.null();
Copy link
Contributor

Choose a reason for hiding this comment

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

to help prevent this, add -a code to the lab command - it'll throw errors if not all the expect(...)s are resolved

@bkendall
Copy link
Contributor

if you want to update the test command now, you can, but LGTM! 👍

rsandor added a commit that referenced this pull request Apr 24, 2015
@rsandor rsandor merged commit 2b373b9 into master Apr 24, 2015
@rsandor rsandor deleted the rsandor/strict-domain-check branch April 24, 2015 21:46
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