Malformed uri:s on Windows

highlevellogic commented 3 years ago

Windows 10, NSS v5.5.6

Added a Trusted App via the data browser and that resulted in a modified card$.ttl file with malformed URI:s such as:

pim:preferencesFile
    <https://rogerfgay.solid.hll.nu:7443%2Fprofile%2Fcard//settings/prefs.ttl>;

Since I'm stuck on Windows, I don't mind trying to fix this, but I'm still a bit of a newbie and would appreciate being pointed to the parts of NSS that are likely to be where the problem is.

bourgeoa commented 3 years ago

You could have a look to request/sharingRequest

highlevellogic commented 3 years ago

resource-mapper, line 87

pathname.split(pathUtil.sep).map((component) => encodeURIComponent(component)).join('/')

Does anyone know why the component should be URI encoded? This is where forward slashes are replaced by %2F.

bourgeoa commented 3 years ago

@highlevellogic this is based on the URL specification https://www.w3.org/Addressing/URL/url-spec.txt and https://www.w3.org/Addressing/URL/url-spec.txt

On the other side files are not limited to ascii To preserve compatibility the only way to resolve that is encode/decode components when going file <--> URI resource.

highlevellogic commented 3 years ago

Shouldn't it be encodeURI() instead of encodeURIComponent()? That would preserve the forward slashes.

I'm using ASCII so did a simple test by not encoding. The problem described here goes away then.

highlevellogic commented 3 years ago

Using encodeURI(), I just added https://шеллы.example.net as a trusted app without problem. It appears in card$.ttl exactly as https://шеллы.example.net Nothing else is malformed as in the first post. Still has relative path for preference and index files.

TallTed commented 3 years ago

Tangent: Please don't use somewhere.be or the like (which might exist, now or in the past or future) where example.be (which should never exist) is appropriate. (See RFC6761.)

highlevellogic commented 3 years ago

Alternatively, trying to see this as a Windows only problem ... just replace pathUtil.sep with '/' :) pathname.split('/').map((component) => encodeURIComponent(component)).join('/')

But I still don't know why there's any need to encode normal URI components such as / and : and ? etc.

bourgeoa commented 3 years ago

Alternatively, trying to see this as a Windows only problem ... just replace pathUtil.sep with '/' :)

But is a windows only because all pod are actually created and working on multiple servers.

You do not encode / because you use pathname.split('/').

highlevellogic commented 3 years ago

Yes. Explicit use of '/' works for Windows too whereas pathUtil.sep (\ on Windows) doesn't.

I'm still wondering about the first solution though. I don't see why encodeURIComponent() is appropriate. Why encode basic parts of URI:s such as / , :, ?, and so on?

Here's a point that may make my questions seem less confusing. I was getting / converted to %2F .... would that ever make sense? (regardless of os)

Using '/' instead of pathUtil.sep is a Windows only solution. But is it the right one?

highlevellogic commented 3 years ago

What would be wrong with doing it like this? (os independet)

pathname.split('/').map((component) => encodeURI(component)).join('/')

bourgeoa commented 3 years ago

As I can see by googling this is a strong recommendation https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent

highlevellogic commented 3 years ago

How is this lesser? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURI

bourgeoa commented 3 years ago

I'm sorry I am not a coder can't help you more.

Have you now a working solution for windows. Are your changes only in resource mapper.js ?

highlevellogic commented 3 years ago

Yes. I can get it to work with only changes is resource mapper. I'm still a newbie but trying to extend my concern to the overall project, I think the questions I'm raising now have to do with striking the right chord with standards.

Aside from that, I'll probably spend the weekend working with aplications I'm trying to build. Hope to have one or two open-sourced next week.

highlevellogic commented 3 years ago

The data browser seems to work pretty well now on Windows. I've submitted PR #1535

highlevellogic commented 3 years ago

My curiosity about this line of code hasn't died. I've even been looking at various uri schemes. It would be more efficient to use: encodeURI(pathname).join('/') rather than pathname.split(pathUtil.sep).map((component) => encodeURIComponent(component)).join('/') So long as the result is correct for all cases. I don't see why it wouldn't be.

bourgeoa commented 3 years ago

I created a pod in your server and true it seems to work quite well with the dataBrowser and solid-ide creating folder, files, managing acl, zip/unzip ... That's a wonderful improvement.

The only bug found is creating chat either from any folder or from profile/card#me returns 500 What NSS version are you using ? Your server says v5.5.2 ? Sorry just checked a header, in fact you are running v5.5.6 May be you could try v5.6.0 to see if the 500 error is still there.

highlevellogic commented 3 years ago

Thank you. Now running v5.6.0

highlevellogic commented 3 years ago

Do I need to do anything to enable websockets?

bourgeoa commented 3 years ago

Path must start with hostname (/bourgeoa.solid.hll.nu)

did you change anything ?

Do I need to do anything to enable websockets?

not to my knowledge

highlevellogic commented 3 years ago

Path must start with hostname (/bourgeoa.solid.hll.nu)

did you change anything ?

Oops! Little mistake when I upgraded. Fixed it. It's correct in my fork - i.e. before the PR.

highlevellogic commented 3 years ago

As follow up to path.sep v / noticed fetch of data/solid.hll.nu/index.html has a rootPath with back slashes. I believe that console log is from line 406 in lib/ldp.js The read doesn't go through resource-mapper. But because JavaScript adapts automatically it still works.

highlevellogic commented 3 years ago

@prefix : <#>.
@prefix ldp: <http://www.w3.org/ns/ldp#>.
@prefix solid: <http://www.w3.org/ns/solid/terms#>.
@prefix foaf: <http://xmlns.com/foaf/0.1/>.
@prefix pim: <http://www.w3.org/ns/pim/space#>.
@prefix pro: <./>.
@prefix n0: <http://www.w3.org/ns/auth/acl#>.
@prefix inbox: </inbox/>.
@prefix bou: </>.
@prefix n: <http://www.w3.org/2006/vcard/ns#>.
@prefix schem: <http://schema.org/>.

pro:card a foaf:PersonalProfileDocument; foaf:maker :me; foaf:primaryTopic :me.

:me
    a schem:Person, foaf:Person;
    n:fn "Alain Bourgeois";
    n:organization-name "individuel";
    n:url
            [
                a n:WebID;
                n:value "https://bourgeoa.solidcommunity.net/profile/card#me"
            ];
    n0:trustedApp
            [
                n0:mode n0:Append, n0:Control, n0:Read, n0:Write;
                n0:origin <https://jeff-zucker.github.io>
            ],
            [
                n0:mode n0:Append, n0:Control, n0:Read, n0:Write;
                n0:origin <https://solidcommunity.net>
            ],
            [
                n0:mode n0:Append, n0:Control, n0:Read, n0:Write;
                n0:origin <https://www.bourgeoa.ga>
            ];
    ldp:inbox inbox:;
    pim:preferencesFile </settings/prefs.ttl>;
    pim:storage bou:;
    solid:account bou:;
    solid:privateTypeIndex </settings/privateTypeIndex.ttl>;
    solid:publicTypeIndex </settings/publicTypeIndex.ttl>;
    foaf:name "Alain Bourgeois".

highlevellogic commented 3 years ago

Are you adding the port number when trying to create a new directory?

solid:handlers PUT -- Error creating directory: Error: Path contains invalid characters: C:/programs/solid/data/bourgeoa.solid.hll.nu/IndividualChats/bourgeoa.solid.hll.nu:7443

It got created anyway: C:\programs\solid\data\bourgeoa.solid.hll.nu\IndividualChats\bourgeoa.solid.hll.nu%3A7443

You also have: C:\programs\solid\data\bourgeoa.solid.hll.nu\IndividualChats\%3A7443

bourgeoa commented 3 years ago

The only bug found is creating chat either from any folder or from profile/card#me returns 500

Yes I was diving to this problem. Chat is not working because of the path name used which is the pod name that includes the port.

This an incompatibility to resolve with mashlib or resource-mapper.js

highlevellogic commented 3 years ago

: is reserved in Windows .. not allowed in directory / filenames. It's explicitely dealt with in /node-modules/fs-extra/lib/mkdirs/make-dir.js

const checkPath = pth => {
  if (process.platform === 'win32') {
    const pathHasInvalidWinCharacters = /[<>:"|?*]/.test(pth.replace(path.parse(pth).root, ''))

    if (pathHasInvalidWinCharacters) {
      const error = new Error(`Path contains invalid characters: ${pth}`)
      error.code = 'EINVAL'
      throw error
    }
  }
}

(I guess that was your point. This isn't where to fix it. :)

bourgeoa commented 3 years ago

Is %encode/decode depending on the os a solution ? I think the solution should not impact the actual Pod's on Linux.

If you try it I could go on further testings.

highlevellogic commented 3 years ago

Windows disallows a different set of characters for directory / filenames. Colon is not allowed.

As workaround, I'm thinking I should set this up to run on port 443. Then we should have WebID:s etc. without port number ... am I right? Should chat work then?

Quick try and there's some confusion about the validity of my certificates. It might be tomorrow before I have it sorted.

Oddly enough, 7443 still works. Certbot complained that the certificates are still good.

bourgeoa commented 3 years ago

It needs to work with any port so a solution is for windows os to convert path colon to %3A and reverse to build URI

highlevellogic commented 3 years ago

It needs to work with any port so a solution is for windows os to convert path colon to %3A and reverse to build URI

Understood. But that seems like another problem. Am I correct that the current system just wants to use : because that works on Linux? (As you were able to create directories with %3A). To get it to work with any port on Windows would then mean fundamentally changing something in Solid. I'd have to start by finding out where in the code to address it and then sell the idea that I'm breaking things for the sake of Windows. (Unless I can be too clever :)

At this point, if testing can be concluded on what's been done already, that would be a plus. Then maybe I can look into the next step if I can figure out where in the code to address it.

highlevellogic commented 3 years ago

Context might be helpful -- I've never even tried to use chat. It may help if I can see how things get set-up on Linux .. directory and file names. For all I know, the fix might be done in /node-modules/fs-extra/lib/mkdirs/make-dir.js ... which I've already found.

bourgeoa commented 3 years ago

@highlevellogic

It needs to work with any port so a solution is for windows os to convert path colon to %3A and reverse to build URI

The chat with me being a new function (that the one creating a folder based on pod's name) and nearly nobody using there own pod with a port # 443. Solid may accept that resource mapper simply removes or replace with _ path containing invalid windows characters.

I'm using VirtualBox and Ubuntu 20.x and visual studio code to have a linux environnement on my windows machine. Do you have an account on gitter.im where are all solid chats, we could continue in private. My pods are running using docker on my synology.

highlevellogic commented 3 years ago

I'm an old guy, and I think living in the same time-zone as you. Early to bed. Early to rise. Can't avoid calling it quits for the day at a reasonable time. Could try connecting with you real-time tomorrow if you're available.

But you might have just suggested what I need to know. It's only a matter of a directory name with colon port?

That would make my first guess that the check for colon wouldn't be limited to Windows. Instead of rejecting, it would encode and the result would be the same on any os.

bourgeoa commented 3 years ago

It's only a matter of a directory name with colon port?

Yes

... and the result would be the same on any os.

I can see one draw back to that. It is what solid/solid-rest is at creating a pod from any file-system. In that case it doesn't seem a good idea to reject a valid file

highlevellogic commented 3 years ago

chat via home icon +

You don't seem to have any workspaces. You have 1 storage spaces. TypeError: e.id is not a function

highlevellogic commented 3 years ago

Try chat via dir icon +

Meetings might work too, but I haven't found the button for that. :)

highlevellogic commented 3 years ago

bookmarks.ttl suddenly appeared in my profile and is seen for both public and private on the home page. I can't find the big "Create Chat" button anymore.

highlevellogic commented 3 years ago

You might need to log out and log back in again for full effect.

bourgeoa commented 3 years ago

@highlevellogic I don't know if you are still working on solid for windows. I have been able to run tests on windows using github actions in replacement of Travis on github that do not work anymore for solid :

in a forked instance of node-solid-server node-solid-server (you can have a look to bourgeoa/node-solid-server)
create a branch by clicking on master, call it windows-fix
go to that branch, select the actions menu and choose node.js CI

edit the proposed code to :


# This workflow will do a clean install of node dependencies, build the source code and run tests across different versions of node
# For more information see: https://help.github.com/actions/language-and-framework-guides/using-nodejs-with-github-actions

name: CI

on: push: branches: [ windows-fix ] pull_request: branches: [ windows-fix ]

jobs: build:

runs-on: ${{ matrix.os }}

strategy:
  matrix:
    node-version: [10.x, 12.x, 14.x]
    os: [ubuntu-latest, windows-latest]

steps:
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node-version }}
  uses: actions/setup-node@v1
  with:
    node-version: ${{ matrix.node-version }}
- run: npm ci
# test code
- name: standard
  if: matrix.os == 'ubuntu-latest' 
  run: |
    npm run standard
    npm run validate
- run: npm run nyc
# Test global install of the package
# - run: npm pack .
# - run: npm install -g solid-server-*.tgz
# Run the Solid test-suite
# - run: bash test/surface/run-solid-test-suite.sh


- `npm run standard` and `npm run validate` do not work in windows, that's why I kept `ubuntu-latest`
- results : 
   - few remaining errors (2 or 3 end of line, or a few path with \ or \\ in lieu of /
   - except on `PATCH with n3` where nothing succeeds (PATCH with `application-sparql` works perfectly

highlevellogic commented 3 years ago

Thanks Alain. I'll look at this tomorrow. I haven't left the planet. I've been trying to get my node proxy to work with Solid.

highlevellogic commented 3 years ago

First time running such tests ... need to be clear first. I have followed your instructions up to the point of running tests. I have 'Create node.js.yml' in highlevellogic/node-solid-server/.github/workflows/ .... master Tried to npm run standard but got error: ENOENT: no such file or directory, open 'C:\programs\solid\package.json'

So now I should then run some kind of install from github? And that will replace my existing installation with a new one?

I don't yet have any experience using a github tools, even though Github Desktop, Git Bash, and Git GUI are installed. I also have a bunch of edited files right now doing various checks and experments.

What did happen the last time you tried running on mine? I removed ":7443" explicitely from all incoming request.urls I did not change the rejection for windows reserved characters (but the port should not be included when the code runs that.)

highlevellogic commented 3 years ago

Looking at these instructions (right stuff?) https://docs.gitlab.com/ee/ci/quick_start/ ... doesn't look like I have any runners. . reading on.

bourgeoa commented 3 years ago

Sorry I don't know for gitlab

(you can replace everywhere windows-fix by what you want, that one exist ans contain your first changes to ressource-mapper.js)

I went to your fork and you created the github action in master not in windows-fix branch where I proposed to make your PR not to mix with master
Your github action in master is wrong because when adding a PR in master, github is asked to run the tests on the windows-fix branch.
So what to do now is :
- Add a github action in windows-fix
- Tests are run when you add a PR in windows-fix branch
- Whenever you make changes in any file when being in windows-fix branch github will ask you to commit to windows-fix and will run the github actions. Here : run tests against windows-fix branch (you can also use git locally and push a commit to the remote github)

I haven't been able to run tests locally that is why I tried github action and it worked Hope it helps Don't hesitate to ask until it works.

highlevellogic commented 3 years ago

I have a windows-fix branch and had that selected when I clicked on actions. i.e. actions -> New Workflow Node.js Button : Set up this workflow npm ci npm run build --if-present npm test

This is where I need some help getting it right it seems. When I did that, the yaml file was in the master.

highlevellogic commented 3 years ago

When I start the commit for it, there's where I chose the master. Two choices exist: Commit directly to the master branch. Create a new branch for this commit and start a pull request. Learn more about pull requests.

I had already created windows-fix. It's getting late now, so if dumb questions are allowed we might get farther on this today .. .. without my experimenting too much. What should I do?

otherwise ... tomorrow is another day and I'll try again.

bourgeoa commented 3 years ago

An other way is create the file manually. When you are in the windows-fix branch

goto add file
create.github/workflows/node.js.yml
then paste the content
commit to the windows-fix branch

highlevellogic commented 3 years ago

Still facing Commit directly to the windows-fix branch. Create a new branch for this commit and start a pull request. Learn more about pull requests. .... selecting the latter with windows-fix says: Will be created as windows-fix-1

leads to Open a pull request

highlevellogic commented 3 years ago

I now have node.js.yml #1 Open highlevellogic wants to merge 1 commit into windows-fix from windows-fix-1

Some checks were not successful x build 12x ubuntu-latest

bourgeoa commented 3 years ago

OK then click on x build 12x ubuntu-latest

and you will find this message on standard step line 10

/home/runner/work/node-solid-server/node-solid-server/lib/resource-mapper.js:9:7: 'pathUtil' is assigned a value but never used.

highlevellogic commented 3 years ago

6s Run npm run standard

solid-server@5.6.0 standard /home/runner/work/node-solid-server/node-solid-server standard '{bin,examples,lib,test}/*/.js'

standard: Use JavaScript Standard Style (https://standardjs.com) /home/runner/work/node-solid-server/node-solid-server/lib/resource-mapper.js:9:7: 'pathUtil' is assigned a value but never used. npm ERR! code ELIFECYCLE npm ERR! errno 1 npm ERR! solid-server@5.6.0 standard: standard '{bin,examples,lib,test}/**/*.js' npm ERR! Exit status 1 npm ERR! npm ERR! Failed at the solid-server@5.6.0 standard script. npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in: npm ERR! /home/runner/.npm/_logs/2020-12-10T15_07_24_475Z-debug.log Error: Process completed with exit code 1.

nodeSolidServer / node-solid-server

Malformed uri:s on Windows #1534