-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Change domain blocks to automatically support subdomains #11138
Conversation
If a more authoritative domain is blocked (example.com), then the same block will be applied to a subdomain (foo.example.com)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the inline comments, it looks like blocks will affect newly-discovered accounts correctly, but not existing accounts on a subdomain of the blocked account. Unblocking domains would have the same issue.
def rule_for(domain) | ||
return if domain.blank? | ||
|
||
uri = Addressable::URI.new.tap { |u| u.host = domain.gsub(/[\/]/, '') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is it for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normalizing the domain just in case before the comparison, because the domain stored in the domain_blocks table is normalized.
app/models/domain_block.rb
Outdated
|
||
segments.size.times do |i| | ||
variants << segments[i..-1].join('.') unless i == segments.size - 1 | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there a more readable way to write that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is I haven't thought of it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variants = segments.each_with_index.map { |_, i| segments[i..-1].join('.') }
is a little nicer.
I have thought of this, but I don't know how to approach this. A wildcard SQL query? For a destructive action? That seems... dangerous. At the very least this PR as-is would protect against servers circumventing existing blocks |
@ThibG Okay, I believe I have addressed it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % nits
app/models/domain_block.rb
Outdated
|
||
segments.size.times do |i| | ||
variants << segments[i..-1].join('.') unless i == segments.size - 1 | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variants = segments.each_with_index.map { |_, i| segments[i..-1].join('.') }
is a little nicer.
Will this apply to blocks already existing before the update containing this feature? I'd be in favor of that, as quite a lot of instances already did a preemptive block of $youknowwhat, but develop.:hankey:.com just popped up. |
yes, this is applied to already existing blocks (theoretically)
…On Fri, Jun 21, 2019 at 6:44 PM Trolli Schmittlauch < ***@***.***> wrote:
Will this apply to already existing blocks? I'd be in favor of that, as
quite a lot of instances already did a preemptive block of $youknowwhat,
but develop.💩.com just popped up.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#11138>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABZCV4J4D33RQBVJZEYO6DP3VKVTANCNFSM4H2RLH6Q>
.
|
No, there's still something missing, it won't block already-known accounts from a subdomain matching an already-existing domain block. A migration script or something would be needed. |
…cally support subdomains * Change domain blocks to automatically support subdomains If a more authoritative domain is blocked (example.com), then the same block will be applied to a subdomain (foo.example.com) * Match subdomains of existing accounts when blocking/unblocking domains * Improve code style
any updates on this? edit: context |
If a more authoritative domain is blocked (example.com), then the
same block will be applied to a subdomain (foo.example.com)