Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

More CLI settings for IPFS API #4608

Merged
merged 9 commits into from
Feb 24, 2017
Merged

More CLI settings for IPFS API #4608

merged 9 commits into from
Feb 24, 2017

Conversation

maciejhirsz
Copy link
Contributor

  • Allow to set different address binding via --ipfs-api-interface.
  • Allow to set a list of Host headers to filter via --ipfs-api-hosts.
  • Allow to set CORS via --ipfs-api-cors.
  • Requests will be killed if invalid Host or Origin header is sent to avoid side effects.
  • DRY-ed up and removed some unnecessary cloning in parity/configuration.rs.

@maciejhirsz maciejhirsz added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Feb 20, 2017
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Couple of grumbles.

ipfs/src/lib.rs Outdated

pub fn new(cors: Option<Vec<String>>, hosts: Option<Vec<String>>, client: Arc<BlockChainClient>) -> Self {
fn origin_to_header(origin: String) -> AccessControlAllowOrigin {
if origin == "*" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should handle null as well (file:/// and sandboxed iframes)

ipfs/src/lib.rs Outdated
pub struct IpfsHandler {
/// Response to send out
out: Out,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why put those new lines? I think I a compact form more.


cors_domains.iter().any(|domain| match *domain {
AccessControlAllowOrigin::Value(ref allowed) => origin == allowed,
AccessControlAllowOrigin::Any => true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Null?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already fixed, you just can't see it on the diff here (it's one line below).

}
}

if let Some(cors_header) = cors::get_cors_header(&self.cors_domains, &self.origin) {
res.headers_mut().set(cors_header);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should also send Access-Control-Allow-Methods, Access-Control-Allow-Headers and Vary: Origin headers.

Copy link
Contributor Author

@maciejhirsz maciejhirsz Feb 22, 2017

Choose a reason for hiding this comment

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

Access-Control-Allow-Methods and Access-Control-Allow-Headers are only necessary in preflight requests, currently non-GET requests get killed outright so there is no need for them. I've added Vary.

ipfs/src/lib.rs Outdated
port: u16,
interface: String,
cors: Option<Vec<String>>,
hosts: Option<Vec<String>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Formatting is odd here.

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 23, 2017
@maciejhirsz maciejhirsz added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Feb 23, 2017
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 24, 2017
@gavofyork gavofyork merged commit f97e775 into master Feb 24, 2017
@gavofyork gavofyork deleted the mh-ipfs branch February 24, 2017 09:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants