This repository has been archived by the owner on Aug 23, 2020. It is now read-only.

Allow IXI Modules to dictate response's content-type. #743

Open

brunoamancio wants to merge 12 commits into iotaledger:dev from brunoamancio:dev

brunoamancio commented May 4, 2018 •

edited

Loading

Description

This PR allows IXI Modules to decide the content-type they wish to send to the client. The idea is that clients might want to, for example, load web content directly on their browser, get data in XML format, images, or whatever the IXI module serves.

This PR was created for issue #615

Checklist:

My code follows the style guidelines for this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
New and existing unit tests pass locally with my changes

brunoamancio added 3 commits

May 4, 2018 12:24


          IXI modules can define the response type.

86a5189


          Fixed wrong variable name.

09d7a11


          Made method getResponseContentType static.

ad0b1d2

Author

brunoamancio commented May 4, 2018 •

edited

Loading

To test the IXI part, you can take https://github.com/brunoamancio/TangleNet , an IXI module which provides the client with web content from the Tangle.

Just query from the browser (get request):
http://localhost:14265/?command=TangleNet.view&tail=
parameter tail = Tail transaction of bundle with web content

Example:

brunoamancio added 2 commits

May 5, 2018 00:37


          Changed code according to codacy-bot's suggestions.

ecafa1f


          Added missing bracket =(

43049df

iotaledger deleted a comment

iotaledger deleted a comment

iotaledger deleted a comment

iotaledger deleted a comment

iotaledger deleted a comment from brunoamancio

iotaledger deleted a comment

iotaledger deleted a comment from brunoamancio


          Changing code style for bot's complaint.

08ab8d5

Author

brunoamancio commented May 4, 2018 •

edited

Loading

It might also be possible to use gson to convert the query parameters to json. That way I can save some code. But for now I haven't tried it yet. I can analyse the possibility to improve that. (DONE)

iotaledger deleted a comment

iotaledger deleted a comment from brunoamancio

iotaledger deleted a comment

iotaledger deleted a comment

iotaledger deleted a comment

iotaledger deleted a comment from brunoamancio

GalRogozinski suggested changes

View reviewed changes

Contributor

GalRogozinski left a comment

Please review change requests

src/main/java/com/iota/iri/service/API.java Outdated

+                      Set<String> keySet = queryParameters.keySet();
+                      for (String key : keySet) {
+                          String param = queryParameters.get(key).getFirst();

Contributor

GalRogozinski May 6, 2018

I don't like concatenated gets...
Can you seperate to two lines?

src/main/java/com/iota/iri/service/API.java Outdated

+                      for (String key : keySet) {
+                          String param = queryParameters.get(key).getFirst();
+                          queryParamsBody += "\"" + key + "\" : " + "\"" + param + "\","; // Json property

Contributor

GalRogozinski May 6, 2018

We are still using Java 8,
So can you use StringBuilder?

Author

brunoamancio May 6, 2018 •

edited

Loading

I've used gson to convert the parameters. It looks cleaner now.

src/main/java/com/iota/iri/service/API.java Outdated

                           }
                           if (instance.configuration.string(DefaultConfSettings.REMOTE_LIMIT_API).contains(command) &&
-                                  !sourceAddress.getAddress().isLoopbackAddress()) {
+                                  !exchange.getSourceAddress().getAddress().isLoopbackAddress()) {

Contributor

GalRogozinski May 6, 2018

concatenated gets...

Author

brunoamancio May 6, 2018 •

edited

Loading

I will do that, although the extra variable will be set even when the first condition isn't met. That means we are trading performance for maintainability.

src/main/java/com/iota/iri/service/API.java

                               return AccessLimitedResponse.create("COMMAND " + command + " is not available on this node");
                           }
                           log.debug("# {} -> Requesting command '{}'", counter.incrementAndGet(), command);
                           switch (command) {
                               case "storeMessage": {
+                                  if(!iotaAPIHeaderDefined(exchange)){

Contributor

GalRogozinski May 6, 2018

Does this always appear?
Instead of copy/pasting it can you write it once before the switch?

Author

brunoamancio May 6, 2018 •

edited

Loading

The idea is to not require the API version header for calls to IXI modules. Due to the current use of a switch we have 3 options:
1 - Paste the check for every API call, except call to IXI modules (which is what I've done)
2 - Refactor the current logic to use command pattern
3 - Use a Map structure to map command names to API calls (kind of a command pattern)

I can refactor the existing command structure, but that should be done in another PR.

src/main/java/com/iota/iri/service/API.java Outdated

+                      String response = null;
+                      if(res instanceof IXIResponse){
+                          final String content = ((IXIResponse)res).getContent();
+                          if(content != null && !content.equals("")){

Contributor

GalRogozinski May 6, 2018

Should " " pass?

Use StringUtils.isNotEmpty() or StringUtils.isNotBlank() according to your answer

src/main/java/com/iota/iri/service/dto/IXIResponse.java Outdated

+                  }
+                  public String getResponseContentType() {
+                      Object fieldObj = getResponseMapper().get("contentType");

Contributor

GalRogozinski May 6, 2018

concatenated gets

src/main/java/com/iota/iri/service/dto/IXIResponse.java Outdated

+                  public String getResponseContentType() {
+                      Object fieldObj = getResponseMapper().get("contentType");
+                      String fieldValue = fieldObj == null || "".equals(fieldObj) ? getdefaultContentType() : fieldObj.toString();

Contributor

GalRogozinski May 6, 2018

Should " " pass?

Use StringUtils.isNotEmpty() or StringUtils.isNotBlank() according to your answer

src/main/java/com/iota/iri/service/dto/IXIResponse.java Outdated

+                  public String getContent() {
+                      Object fieldObj = getResponseMapper().get("content");
+                      String fieldValue = fieldObj == null || "".equals(fieldObj) ? null : fieldObj.toString();

Contributor

GalRogozinski May 6, 2018

Should " " pass?

Use StringUtils.isNotEmpty() or StringUtils.isNotBlank() according to your answer

brunoamancio added 3 commits

May 6, 2018 17:34


          Changed code to comply with GalRogozinski's suggestions.

5bab03b


          Further changes to comply with GalRogozinski's suggestions

58be2c5


          Separated getters in IXIResponse.

716214d

iotaledger deleted a comment

iotaledger deleted a comment

brunoamancio mentioned this pull request

Api command functional routing #392

Open

Contributor

footloosejava commented May 7, 2018

@brunoamancio Pretty cool mate!
What do you see as the end-user application? When would someone request web pages from the network?

Author

brunoamancio commented May 7, 2018 •

edited

Loading

@footloosejava With these changes, accessing tangle-only and hybrid-hosted (tangle/internet) web content will be a reality. I have already implemented a simple IXI module (https://github.com/brunoamancio/TangleNet) to allow nodes provide that to clients sending requests directly from their browsers.

I plan on developing it further to make this process as user-friendly as possible.

brunoamancio added 2 commits

May 13, 2018 16:11


          Merge branch 'dev' into dev

c0a23a2


          Changed IOUtils to IotaIOUtils to satisfy changes in main dev branch

4f8551f

iotaledger deleted a comment

iotaledger deleted a comment

iotaledger deleted a comment from brunoamancio

iotaledger deleted a comment from yovendielmundo

iotaledger deleted a comment from brunoamancio

iotaledger deleted a comment from brunoamancio

iotaledger deleted a comment from yovendielmundo

iotaledger deleted a comment from brunoamancio


          Added variable in method getRequestBody in API.java

c4094f8

iotaledger deleted a comment

iotaledger deleted a comment

GalRogozinski requested a review from alon-e

May 13, 2018 16:06

Contributor

GalRogozinski commented May 13, 2018

@alon-e
Can you also review please?

Author

brunoamancio commented May 13, 2018 •

edited

Loading

I will refactor the command logic in another PR. Until then, this PR will be left as is.
UPDATE: I've seen @GalRogozinski's comment in the command-related PR (#392 (comment)). I'm also waiting for a response.

GalRogozinski added C-IXI T-Enhancement labels

Contributor

alon-e commented May 16, 2018

@brunoamancio I'll review this shortly.
a few remarks:

have you tested backward compatibility with existing IXI modules?
examples:
https://github.com/bluedigits/nstats
https://github.com/iotaledger/Snapshot.ixi
https://github.com/iotaledger/MAM.ixi
adding disableHtmlEscaping() is alarming, could you elaborate on the need and how do you mitigate attacks against input validation? for example, reflective XSS via "COMMAND " + command + " is not available on this node"? (I'll be looking into this as well)
the iotaAPIHeaderDefined() additions are too much to swallow IMHO. the idea is to make IRI more readable and maintainable, there must be a better way to do that.
I understand the reason is to not make this PR more complex, but in that case, we should first change the command routing (a la Api command functional routing #392) and then redactor this PR. - i.e. use this as a PoC, but not merge it just yet.

Author

brunoamancio commented May 16, 2018 •

edited

Loading

Answering @alon-e questions:

1 - In case the IXIResponse does not have the specific fields reserved for specifying the content and content-type of the response, the behavior is exactly as before.

That means, only if code is like the following will this feature be used:

... javascript code of IXI module

return Response.create({ contentType: "specific-content/type", content : "response in the response content-type format"})
...

That is done in the new method convertResponseToClientFormat in API.java

Also, the method getResponseContentType makes sure the default content-type is json, like it is today. Only if it is explicitly defined by the IXIModule (in its response), will the response type be changed.

Anyway, I can run those and tell you the results in a few days as well.

2 - The reason is that html content (and any other content format) is converted from the javascript-generated response from the IXI Module to IXIResponse using gson, which makes a desired response with html to be unreadable by the IRI client, since it's escaped.

More details on gson in the API.java (where escaping is disabled):

When gson is used to convert query parameter strings to json, the reason is to match the post request body structure, which is a string with json content. I'll discuss this in the respective point below, since any issues should be handled the same way.
When the request body has unescaped values, the only case where reflective XSS is "possible" is the example you gave: ErrorResponse.create("Command [" + command + "] is unknown"). Other usages of parameters always cast/convert to specific types before using values.
In this case, it is already possible (in the current code) to send in whatever one wants to in the post request body. But, since it is only used to send an error message back to the client who sent the request, there is no issue. If it was an SQL query, for example, that would be a huge problem, but the java byte code itself can't be injected.
Although, a way to prevent that, if there was an issue. The open source OWASP Enterprise Security API:
String safe = ESAPI.encoder().encodeForHTML( command );
When gson is used to convert a response to client format (method convertResponseToClientFormat(...)) there is no issue, since the client will get the response content in the expected format and knows how to handle it. It's just a conveyor belt from the IXI Module in this case.

There are no other usages of gson in the API.java class

*Alternative: It would be possible to explicitly unescape IXIResponses before sending it to the client. But that would mean IRI knows what should be abstract to it (the content-type).

3 - I completely agree with that. I don't currently have time for the refactoring of the command logic, but if nothing is committed until I have time, I will do it myself in a separate PR and after the merge this PR can be updated with the better structure.

Author

brunoamancio commented May 16, 2018 •

edited

Loading

Please don't delete this comment
Note to myself:
In class IXIResponse methods getContent() and getResponseContentType(): Ensure variables are empty when properties "content" and "contentType" are not strings in IXI Module (they might be object or number).

Contributor

GalRogozinski commented May 29, 2018

@brunoamancio
Did you perform the tests @alon-e asked you to?

Author

brunoamancio commented May 29, 2018 •

edited

Loading

Hi @GalRogozinski . I have not, yet. I'm really busy at work (.NET developer here) these last weeks and didn't have much free time after working hours either.

Anyway, I will perform those tests asap, possibly this weekend.

Update:
I'm working on Hercules, so I can't implement that. it'd be great if someone else could :)

xn3cr0nx pushed a commit to xn3cr0nx/iri that referenced this pull request


          Features: Allow IXI Modules to dictate response's content-type. (iota…

bb852b4

…ledger#743)

Updates old feature pull request improving X-IOTA-API-Version check management

xn3cr0nx mentioned this pull request

Allow IXI to manage more than POST HTTP requests #1385

Open

3 tasks

xn3cr0nx commented Mar 20, 2019

Hi everyone, I'm interested in this topic. I opened a new PR (#1385) with some little improvements to keep discussing about it and I hope we could be able to eventually implement this feature.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

C-IXI T-Enhancement