Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

Allow notifies coming from external hosts #643

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

paoloaga
Copy link

Handy to use on a computer that doesn't host the bitcoind node (like a RPi).

@AdamISZ
Copy link
Member

AdamISZ commented Oct 21, 2016

All PRs against develop branch please.

@paoloaga
Copy link
Author

Ok, next time I will push there. Anyway this is just a very brief patch (just a string in the code). Not sure if it's worth it to make another PR. If you think it's valid, just insert it in the next release. Thanks.

@chris-belcher
Copy link
Collaborator

It should be possible to change the PR destination without closing it and opening a new one.

@paoloaga paoloaga changed the base branch from master to develop October 24, 2016 15:35
@paoloaga
Copy link
Author

I found the feature, thanks. Is it all right now?

@AdamISZ
Copy link
Member

AdamISZ commented Oct 24, 2016

OK, cool, @chris-belcher do you ACK? (sorry for trivial question but I know you actually thought about this, I didn't :)) If you merge please use github-merge.py to sign the merge commit, else I can do it.

@adlai
Copy link
Contributor

adlai commented Oct 24, 2016

utACK

This does make scripts slightly more vulnerable to grinding induced by other computers, but I think that people running JM with ports forwarded should know what they're doing. Also, I don't see that this creates dangers beyond merely consuming extra resources, in which case the operator could stop forwarding ports and use an SSH tunnel instead.

Further thoughts, @chris-belcher ?

@chris-belcher
Copy link
Collaborator

Yes I think its good. utACK.

About a year ago we had a vulnerability which involved passing strings from the http server to system(). Since then that part was changed to use urllib which doesn't pose such a serious security risk.

@adlai
Copy link
Contributor

adlai commented Oct 27, 2016

I'm amending my position to NACK, because this does open up more avenues for grinding or even worse vulnerabilities. Off the top of my head I can't think of how to exploit this, but why make life easier for enemies?

I'll ACK a version of this which lets the user configure the binding host, but leaves the default as localhost (or 127.0.0.1, which I believe has the same effect).

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

Successfully merging this pull request may close these issues.

4 participants