Node-solid-server: Users with only acl:Append permission are able to POST ACL files

[angelo-v]

Actual behaviour

Given a container where I have acl:Append permission, but no acl:Control permission
When I POST a file to the container using a Slug header ending with .acl
Then the resource gets created and is considered as a valid ACL

Expected behaviour

Creating ACL resources is only possible with acl:Control permission

Example

Container at path /foo with public write permission containing a resource bar.ttl

Request to create an ACL file for bar.ttl:

POST /foo HTTP/1.1
Content-Type: text/turtle
Link: <http://www.w3.org/ns/ldp#Resource>; rel="type"
Slug: bar.ttl.acl

# ACL content

[csarven]

It is only "considered as a valid ACL" because of the automatic relation with a URI template that's in use (which happens to match the created resource) ie. id acl id.acl.

If bar.ttl exists and has rel=acl to bar.ttl.acl, I agree that this is an NSS bug and it should reject the request to create bar.ttl.acl from user with acl:Append - if it didn't do anything about the Slug suggestion. If it did, then it'll just rename to something "safe".

If however bar.ttl doesn't exist, bar.ttl.acl should be handled as any RDF source - not to be "considered as a valid ACL" unless of course if NSS reserves .acl suffix in resource names for "valid ACL"s. So, it shouldn't allow bar.ttl.acl to be created where it can potentially be linked from bar.ttl.

[justinwb]

If however bar.ttl doesn't exist, bar.ttl.acl should be handled as any RDF source - not to be "considered as a valid ACL" unless of course if NSS reserves .acl suffix in resource names for "valid ACL"s.

I don't think it _should_ be handled as any RDF source. The association of a given resource R to an acl resource A in NSS is based on the presence of '.acl' at the end of a A's name, when the part before the .acl in A's name is the same name as R. If A's name is just '.acl', it would apply to the container that A is a member of.

Consider the creation of a resource named 'foo' in container C. If NSS doesn't ensure that there isn't a foo.acl in C first, anyone that can predict that a file named foo will be created at some point can set the permissions in advance by making foo.acl, even if they don't have control permission on container C.

IMO - the simplest path would be for NSS to reserve the .acl suffix to avoid this. Note this is an NSS problem, not a Solid problem. In Solid you can make any resource names you want, and the server maintains the association of acls. It is _how_ NSS maintains that association that leads to this potential issue.

[csarven]

..unless..
So, it shouldn't allow bar.ttl.acl to be created where it can potentially be linked from bar.ttl.

[csarven]

Fixed by #1419. @angelo-v @justinwb et al, can you also verify?

[scenaristeur]

Hi Guys,
Yesterday I was able to use @jeff-zucker https://github.com/jeff-zucker/solid-file-client to create folders & files .acl but
Today I can't & got a Http 500 ERROR, so i'would like to know if that new fix could be the source of my "BMSE" ( Bad Mistake Search Evening) ???

by default, I have CONTROL to the folder & want to add Authenticated Agents as Submitter (https://spoggy-test.solid.community/public/shighl_test/inbox/)

Here is the code I use

``` let inbox = "https://spoggy-test.solid.community/public/shighl_test/inbox/" let aclInboxContent =@prefix : <#>.
@prefix acl: http://www.w3.org/ns/auth/acl#.
@prefix inbox: <./>.
@prefix c:

:Append
a acl:Authorization;
acl:accessTo <./>;
acl:agentClass acl:AuthenticatedAgent;
acl:default <./>;
acl:mode acl:Append.
:ControlReadWrite
a acl:Authorization;
acl:accessTo <./>;
acl:agent c:me;
acl:default <./>;
acl:mode acl:Control, acl:Read, acl:Write.
:Read
a acl:Authorization;
acl:accessTo <./>;
acl:default <./>;
acl:mode acl:Read.`

let file = inbox+".acl"
await fc.createFile (file, aclInboxContent, "text/turtle") .then (success => {
  this.log = "Created "+file
}, err => {
  this.log = err
  alert(err)
});

```
here is the app i'm working on https://scenaristeur.github.io/agora/
thxs

[jeff-zucker]

It would be relevant to know what content-type you specified in the PUT request that generated the 500 error.

[scenaristeur]

@jeff-zucker
"text/turtle"

await fc.createFile (file, aclInboxContent, "text/turtle")

That worked yesterday. Can you create an .acl file today with solid-file-client on a solid.community pod?

[bourgeoa]

Can you try to do it manually using dataBrowser.
I have done it and it works. If you put your resource address .../inbox/.acl in the dataBrowser you can display the acl content with the text edit icon.

Your acl is wrong prefix foaf: is missing, you are using prefix acl: in lieu of prefix foaf:

You are also missing in Read block acl:agentClass foaf:agent

[scenaristeur]

@bourgeoa sure if I want that the inbox can be read by everyone, i need foaf:agent...
but i general, this is not expected ;-)

I want

so the .acl should be

But my question is does this fix could have any influence to the @jeffz solid-file-client putting .acl or not ? As I said there was no matter yesterday to put that file... Is there a server where that patch has not been applied so i could test ?
Does anyone achieve to put an .acl file after the patch with solid-file-client ?

[scenaristeur]

@jeffz here you can test how I try to create .acl with solid-file-client https://scenaristeur.github.io/agora/acl_test.html
here is my test code https://github.com/scenaristeur/agora/blob/gh-pages/acl_test.html
I think that worked two days ago but maybe i'm wrong ?

[scenaristeur]

confirmed by the same test on @bourgeoa server that did not apply the last patch .
Using solid-file-client
On solid.community : PUT file.ttl -> OK & PUT file.ttl.acl -> KO (but was OK 2 days ago)
On @bourgeoa server : PUT file.ttl -> OK & PUT file.ttl -> OK
So that patch block solid-file-client to write .acl file

[angelo-v]

confirmed it doing a manual PUT

PUT https://[...].solid.community/demo/foo.acl

HTTP/1.1 500 Internal Server Error
[...]

No ACL found for https://[...].solid.community/demo/foo.acl, searched in 
- https://[...].solid.community/demo/foo.acl
- https://[...].solid.community/demo/.acl
- https://[...].solid.community/.acl


Response code: 500 (Internal Server Error); Time: 104ms; Content length: 226 bytes

[angelo-v]

@jaxoncreed
The test you added just did not describe the bug behaviour I reported. Here is a fixed test that reproduces the problem properly and also discloses the observed 500 Status bug

it('user2 should not be able able to post an acl file', function (done) {
      var options = createOptions('/append-acl/', 'user2', 'text/turtle')
      options.headers.Slug = 'some.acl'
      options.body = '<a> <b> <c> .\n'
      request.post(options, function (error, response, body) {
        assert.equal(error, null)
        assert.equal(response.statusCode, 403)
        assert.equal(response.statusMessage, 'User Unauthorized')
        done()
      })
    })

[angelo-v]

I set up tests in my fork that disclose the described behaviour properly (based on the version before the "fix")

https://github.com/angelo-v/node-solid-server/pull/2/files

[jeff-zucker]

@scenaristeur this bug has nothing to do with solid-file-client, it is an NSS bug that occurs regardless of what library does the file writing. When you encounter a problem, you should try to break it down to its simplest form, removing all intermediary software. In this case some plain PUT and POST tests as @angelo-v showed rather than using solid-file-client to write. This helps people identify where the problem is, makes tests easier to reproduce, and saves me from having to check solid-file-client every time there is an NSS bug. If you find that a straight PUT or POST behaves correctly and solid-file-client does not, that's when you should file an issue on solid-file-client. If you need help writing plain PUT and POST tests, let me know and I'll be glad to give you some tips.

[scenaristeur]

@jeffz sorry for disturbing you if the problem is not on solid-file-client client, but I first wanted to share that there I encountered a problem and asked if that patch was the cause. I shared the way / context I encountered it and made a reproductible test.
I didn't know if the problem was on nss or solid-file-client and first needed to have a confirmation. Or not.
Perhaps I made something wrong and that's why I shared you my test to ask first you that I used sfc in a correct way.

As a sfc expert you could tell me if I was wrong or not. If I was not wrong, the problem could be in sfc or somewhere else, but I personally didn't know where.

Then @bourgeoa give me access to a non patched provider and we confirmed that incompatibility between the new patch and sfc. Then @bourgeoa & @angelo-v excluded the sfc responsability .

During 2 days I've tried to alert about a problem and explain as I could understand the cause... and I thought that if I encountered that error, some other user of sfc could encounter it. If that patch was applied, it would break some sfc capabilities and that's why I wanted to alert you personally.

But I'm not a expert in .Acl, authorization, post,put,patch, system...

Really sorry for disturbing you. If you prefer, the next time it happens, I say nothing the patch is applied, and sfc stay broken until someone, one day try to put an Acl file with sfc.
Just want to help 😊

[jeff-zucker]

David, I'm not asking you to stop notifying me if there are problems or asking for my help in tracking them down. You are always welcome to do that. I am asking that when you report a problem, you reduce the problem to the least number of elements and test those.

[scenaristeur]

That's what I did with my knowledge 😊.
But don't know how all your great tools works 😊🥂

[melvincarvalho]

@jeff-zucker I feel your pain. I had to take two days out, including my birthday, for this minor patch too. I feel very disappointed that node solid server would release this as a patch release and it causes disruption in both the server and apps

I'm not beating up on @jaxoncreed here either, he's made it clear he's over loaded and that he contributed a patch and it passed the tests was amazing

Also both @jeff-zucker @scenaristeur and @bourgeoa did nothing wrong here

Inrupt was created with a clear mandate to fix the bugs in the reference implementation. Everyone was on the same page with that. We need jackson to be given the resources he needs to do this. For a proper review (there was none). A proper mapping out of the public API and noting the change. HTTP 500 should not occur in any case, and given that node is single threaded, this could be a cause of the long time outs we have seen (still unclear). Changes in the pubic API require a semver minor version bump. Having a working reference implementation is always more important than branding

Room for improvement @timbl @johnbizguy

Edit: corrected and linked to semver spec, added the http 500 note, and time outs, more constructive text

[jeff-zucker]

For me, there is no question of "doing wrong". I have no doubt @scenaristeur was trying to help and that @jaxoncreed is over-worked. Agreed that more notice and testing time for server changes would help, but as you noted, that takes resources.

[scenaristeur]

@melvincarvalho Happy Birthday 🎂🥂

Thank you all... Now I can PUT some .Acl file with sfc 😊.

[d-a-v-i--]

Thank you to Melvin and others for taking time to express concern and send caution. As always we encourage everyone to post feedback and suggestions — a great example of how Solid benefits from the multiple eyes of an open source community.

When the Inrupt team was notified of an important security flaw we responded as fast as possible to provide a fix; to help as quickly as possible keep data protected. Unfortunately as sometimes happens a rapid response test process was incomplete and broader functionality in the ecosystem was impacted by closing this flaw.

We’re now taking action to achieve more thorough testing procedures before we submit even the most critical fixes to the community for review.

We are appreciative of the contributions from this group and others to Solid — The many innovative applications shown off during Solid World a few weeks ago were very encouraging and we’re pushing to get this flaw resolved ASAP to help get everyone back on track.

[jeff-zucker]

@scenaristeur turned out to be right about this and I was wrong. Solid-file-client was part of the problem here. As I understand it, @jaxoncreed corrected @angelo-v 's original problem of the slug on POST being ignored when posting a .acl allowing those without Control to post .acls. That fix triggered @scenaristeur's issue of 500 on PUT of a .acl because solid-file-client has, in recent versions, been sending a slug with PUT. @jaxoncreed rapidly fixed things on the server end (thanks!) and we will also remove the slug from PUT on the solid-file-client end. Thanks to all for a rapid resolution.

[angelo-v]

No @jeff-zucker NSS _is_ broken. Yes, sending the Slug with PUT is not correct, but still here is work to do as my tests indicate, see https://github.com/angelo-v/node-solid-server/pull/2

rapidly fixed things on the server end

As I understand it currently (still not sure, would need more time to investigate), the changes do not fix the problem, they break just enough of the server to make the bug go away as well and replace it with a 500 response.

[jeff-zucker]

@angelo-v did you see @bourgeoa 's last comment at https://github.com/solid/node-solid-server/pull/1422#issuecomment-614580839 seeming to indicate that the fix works. Not sure if he tried your test though.

[angelo-v]

Thanks @jeff-zucker I was not aware of this new PR, which looks fine. I proposed to add the tests I wrote to make sure it works as expected.

[jaxoncreed]

Hey @scenaristeur, thanks for your patience on this. Your issue really illuminated some spec clarification that you can see talked about more here

We think the best way to proceed is to no longer allow ACLs to be created with POST and a Slug header, but there are other ways you can update your app to create ACLs.

The best example is in the react SDK code. Creating an ACL for a resource or a container comes in 2 steps:

Step one: read the resource header to determine the specified location of the acl:

```javascript=
const parse = require("parse-link-header");
const solidAuthClient = require("solid-auth-client");
const documentUri = "https://example.com/app/inbox/";
const response = await solid.fetch(documentUri, { method: 'HEAD' });
const parsedLinks = parse(response.headers.get('Link'));
const aclUrl = parsedLinks.acl ? parsedLinks.acl.url : '';

**Step two: send a PUT request to the aclUrl**

```javascript=
const solidAuthClient = require("solid-auth-client");
await solidAuthClient.fetch(aclUrl, {
  method: 'PUT',
  headers: {
    'Content-Type': 'text/turtle'
  },
  body: "INSERT YOUR ACL TURTLE CONTENT"
});

Hopefully, that helps out, and sorry for all the confusion.

[jeff-zucker]

Or, to accomplish the same thing with solid-file-client :
javascript let response = await fc.getItemLinks( resourceURI ) await fc.createFile( response.acl, ACLcontent, "text/turtle" )
The first call finds the link information in the resource's header, the second uses PUT to write content to it.

[scenaristeur]

Woh Sorry for all the Brainstorming it causes... But I promise you, Now I'll use something to get Acl Link before posting a new one ;-)
I've just updated to the new solid-file-client & it seems to work on inrupt.net 5.2.4
Thxs all for that good job

[bourgeoa]

I suppose the issue can be closed