Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Add configurable data to HTTP header #138

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

Mykyta
Copy link

@Mykyta Mykyta commented May 13, 2020

What / Why

A proposition to add npm-app-id to the HTTP header of npm tool.

meta.app_id = process.env.APP_ID
```

## Prior Art
Copy link
Contributor

Choose a reason for hiding this comment

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

I’ve never heard of this env var being used in the ecosystem. Can you elaborate on how and where it’s already used?

Copy link
Author

Choose a reason for hiding this comment

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

This value is used in the third-part app Nexus Repository Manager which integrated with npm
Currently to add APP_ID is pretty difficult: use application id in package-lock.json
It would be great just to add some npm env var for an npm project and use this value as APP_ID per npm audit.

Copy link
Author

Choose a reason for hiding this comment

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

@isaacs @ljharb can someone review my RFC and approve my propositions?

@isaacs
Copy link
Contributor

isaacs commented May 15, 2020

Some questions.

Is APP_ID an opaque machine-generated token, or something with meaning that the user sets? If it's machine-generated, can the registry provide it, rather than the CLI?

Can we put it in package.json instead of package-lock.json, since package-lock.json is more ephemeral? (Users sometimes blow the package-lock away to get a fresh install. Maybe they'll do that less in npm v7, but who knows, habits die hard.)

Does it need to be (a) unique to each project, (b) unique to each top level project (but shared among workspaces in a monorepo, for example), (c) shared among each package in a user-defined concept of what an "app" is?

Can we spell it appid instead of APP_ID?

I think it'd be a nice simple thing for a registry to provide a npm-app-id HTTP header in the response if it doesn't get an appid, saying "this is the appid you should use", and then the CLI could just stash it in package.json if it gets one, and repeat it back on subsequent requests. A registry server would have to be smart enough to assign the same appid for each new request in a single session (since the CLI sends a bunch of requests out all in parallel without waiting for their responses), but we have the npm-session-id header, so that would be possible.

And if the user does send an npm-app-id header, then the registry would just echo it back.

This would also allow us to do other interesting things in the future, too, especially if an app could be associated with an org or user, and different types of access attached to that indicator. Since it's user-input, we couldn't trust it on its own, but in combination with a login token, we could say that a given appid associated with your organization can only be installed in your production IP block by certain user accounts, so you don't accidentally push to production when you thought you were doing a staging/local build, or something.

@Mykyta
Copy link
Author

Mykyta commented May 15, 2020

Hi, @isaacs here my answers:
APP_ID sets a user while using the third-party app: Nexus Repository Manager only for npm audit. It is similar to the NODE_ENV and how it is used in Express.

npm audit provides package-lock.json in the request body, so we can't use package.json

(a) - It may be unique to each project.

Yes, we can use 'appid' instead of APP_ID.

We can also provide npm-app-id HTTP header like npm-session-id. It is no matter how it will be fetched: from the package-lock.json or HTTP header. For HTTP header I don't know how much effort it will take to implement such functionality, but maybe it the most correct way from the npm CLI perspective. With metadata, we just need to add one line of code: meta.app_id = process.env.APP_ID

And if the user does send an npm-app-id header

User should set npm-app-id in the env var or in the package.json and after npm audit this value should appear in the HTTP header.

I agree it would be nice to implement it as much as useful for CLI tool and provide more flexibility for npm users.

@isaacs
Copy link
Contributor

isaacs commented May 15, 2020

npm audit provides package-lock.json in the request body, so we can't use package.json

That isn't really relevant. We modify the lockfile data that we send anyway, and have the package.json data readily at hand at that time, so it can reside in either place. I feel like "what app is this" is more appropriate for package.json to define than package-lock.json, which is a definition of the dependency package tree as it's laid out on disk.

Also, we're not likely to add a feature like this in npm v6, and we're considering a much more streamlined API for fetching bulk advisories in npm v7 anyway. So, sky's the limit :)

@Mykyta
Copy link
Author

Mykyta commented May 15, 2020

@isaacs yes, I agree that package-lock.json is not the right place to add such custom variable. I'm not a much familiar with npm tool. So what do you think if we move app-id to the HTTP header (which will appear in npm install / npm audit) and this value will be taken from package.json or env variable?

@Mykyta Mykyta changed the title RFC: Add APP_ID environment variable RFC: Add npm-app-id HTTP header May 19, 2020
@Mykyta
Copy link
Author

Mykyta commented May 20, 2020

@isaacs I've updated RFC. Can someone approve these changes to start working under the implementation?

@ruyadorno ruyadorno added the Agenda will be discussed at the Open RFC call label May 20, 2020
@isaacs
Copy link
Contributor

isaacs commented May 20, 2020

I have no objection to the basic idea, but it does need some work to get over the finish line. (Your continued engagement is encouraged and welcome, but not required, if you have other things to do :)

The next step is to discuss it in our weekly Open RFC meeting (which you're invited to attend!) to quickly discuss some of the ramifications of the change, decide next steps, and eventually fit it into our roadmap. I would like to drive towards some more clarity around implementation, alternatives, and prior art, just to have some confidence that we're making the right choices and not opening the door to supporting a mistake forever. Since this is going to be an interface between the client and server, it's important to get the spec as detailed as possible.

The implementation itself (as proposed, anyway) is relatively straightforward, and I don't expect it'll have much impact for other parts of the system, so it should be pretty easy to get landed once we nail down the spec.

@Mykyta
Copy link
Author

Mykyta commented May 21, 2020

Summary from the OpenRFC Meeting:

  • app-id may be added to the HTTP header and can be provided by a user from the npm command parameter or env variable or npm config file
  • Provide a hook for assigning metadata to an application. Make more general mechanism to add some custom user’s info to the npm commands.

The first idea is very simple to implement and have no risk for npm-registry/npm-tool.
The second one is more generic and allow a user to add some custom metadata which can be used by third-party apps and maybe by npm-tool itself.

@isaacs From my perspective of view, it is not clear how to add such a hook mechanism to the npm tool. Also not clear how a user will add such metadata info: from the npm console or npm config file.
Will be it stored in the package.json/package-lock.json or will it be provided in the HTTP header?

@isaacs
Copy link
Contributor

isaacs commented May 22, 2020

@jlstephens89 You can also watch the full discussion, if you wanna feel like you were there :) https://www.youtube.com/watch?v=W6XbJ5e3xrA

@Mykyta
Copy link
Author

Mykyta commented Jun 2, 2020

Hi @isaacs I've created the short Demo https://youtu.be/aMFbEYgmnEk what I expect to get from this feature and how a user can set up/configure their project. We can discuss it more in the upcoming Meeting.

Several remarks here:

  • A user may use the same app-id along with the different projects (in that case a user can set up app-id in the global .npmrc file). Otherwise, it can be set up along with the project.
    So app-id may not be unique.
  • To make it more generalized (not only Nexus products can use it) we can think about the new field in the packaje.json, for example, user_metadata which value will be injected in the request header. I don't know is it possible in npm tool and what risks it will take.
    Another issue from a user side - how this value can be changed for existing project. While running npm audit npm tool can access only to the package-lock.json. And it is not a good idea to set up some data into this file manually. Thus we also have to provide some general command for it - like npm set metadata which will reflect package.json/package-lock.json
  • Also It would be cool to set user metadata via npm config command, for example:
    npm config set metadata_app-id myAppId
    npm config set metadata_project-name myProjectName
    In that case, I expect to see in the request header following parameters:
    app-id with value myAppId
    project-name with value myProjectName
    Thus the suffix name after the metadata_ will go the to HTTP request header name with the corresponding value.
    But I don't know if it is possible to apply some regex to npm registry to fetch this info or not.

@j-s-3
Copy link

j-s-3 commented Jun 3, 2020

If we can get suggestions on how to make this more generic and benefit the wider npm community (not just those that use NXRM), then I'd be happy to make it a priority and carve out some time for my team work on it.

@Mykyta
Copy link
Author

Mykyta commented Jun 10, 2020

Guys, who can help to find the best approach of implementing custom user metadata into the npm registry. Like I wrote above - the best way is:

  • from a user side write:
    npm config set metadata_app-id myAppId
    npm config set metadata_project-name myProjectName
  • while npm commands like npm audit in the HTTP header get the new field: user_metadata
    with value: app-id=myAppId, project-name=myProjectName
    or
    with the new fields: user_metadata-app-id with value myAppId and so on.

So the main issue/question of how to regex user-metadata_(.[\w|\-]+) from the registry and map these values to HTTP header. Is it possible or we have to find another solution?

@doddi
Copy link

doddi commented Aug 14, 2020

I would like to see if the PR (npm/cli#1674) I have put together satisfies the discussion held during the rfc meeting.
In short, the PR allows a generic way to add metadata to the http headers sent as part of the audit command. I think this was asked during the meeting. The metadata to send is retrieved from a metadata object in the package.json file.

Is there an agenda to (re)discuss rfc's? I would be happy to attend

@doddi
Copy link

doddi commented Aug 27, 2020

I have updated the RFC as discussed at the last meeting. Is this at a point where it can be re-reviewed / merged?

@doddi
Copy link

doddi commented Sep 9, 2020

Its possibly a bit late but could this be added to the agenda (9th September)? Or how can I go about taking this discussion further again? /cc @isaacs

@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Sep 9, 2020
@isaacs
Copy link
Contributor

isaacs commented Sep 16, 2020

Other options to explore, discussed in 2020-09-16 RFC call:

{
  "headers": {
    "app-id": "call it headers instead of metadata"
  }
}
{
  "appId": "top-level field"
}
; project-level .npmrc
headers[]="npm-app-id: a config value"
headers[]="Authorization: custom-thing whatever idk"

@isaacs
Copy link
Contributor

isaacs commented Sep 16, 2020

Leaning towards "add headers list to config", and then we can later add an RFC to put a npm-app-id header from the registry is saved to ./.npmrc by default.

@Mykyta Mykyta changed the title RFC: Add npm-app-id HTTP header RFC: Add configurable data to HTTP header Sep 18, 2020
@doddi
Copy link

doddi commented Oct 14, 2020

@isaacs I started to take a look at the implementation of this RFC to get ahead of the game and I found out that the npmrc already caters for additional header info. It is not as elegant is this proposal but it does suggest that there might be a backwards compatibility problem if this RFC is introduced.

https://github.com/npm/cli/blob/5587ac01ffd0d2ea830a6bbb67bb34a611ffc409/node_modules/npm-registry-fetch/config.js#L23 has an entry for headers.

I tested by creating something like the following in a local .npmrc file:

; headers
headers[]="app_id: test"

note that this is identical to the proposal.

What is sent across the wire is:
"1": "app_id: test"

So, although not ideal, this is something I can actually work with. Additionally, 'fixing' it so that it is a correct key/pair sent across the wire my break anyone already using this capability.

This was tested on the latest version 6. I did not check version 7

@isaacs
Copy link
Contributor

isaacs commented Oct 14, 2020

Ohhhhh ok. I wonder if we can do something like this then?

[headers]
some-key = some value

I'll look more closely into it soon.

@isaacs
Copy link
Contributor

isaacs commented Oct 21, 2020

$ cat .npmrc
foo = bar
[headers]
key = value

otherkey = othervalue
bar = baz

npm/arborist isaacs/do-not-collide-on-matching-peer-set-members tau:~/dev/npm/arborist/headers node@v14.8.0
$ npm config get headers
{ otherkey: 'othervalue', bar: 'baz' }

So this is weird? Seems like we drop the first one for some reason.

@ljharb

This comment has been minimized.

@isaacs
Copy link
Contributor

isaacs commented Oct 21, 2020

Ahhhhh, the problem is that I had a trailing space, which the ini package treats as invalid for some reason, and causes it to skip the line.

Yes, this works as expected, as far as I can tell.

@darcyclarke
Copy link
Contributor

@isaacs @doddi can we close this given that headers looks to already be supported/configurable? Should we instead just document this so the functionality isn't lost in translation?

@doddi
Copy link

doddi commented Nov 18, 2020

@isaacs @doddi can we close this given that headers looks to already be supported/configurable? Should we instead just document this so the functionality isn't lost in translation?

I am happy to close if this same functionality is transferred over to v7. That would be ideal because we are already making use of this feature. However, I think @isaacs expressed some concern that this was not intentional behaviour.

@TysonAndre
Copy link

I am happy to close if this same functionality is transferred over to v7. That would be ideal because we are already making use of this feature. However, I think isaacs expressed some concern that this was not intentional behaviour.

This [headers] functionality still works in v6 but was not transferred over to v7 or v8, for anyone wondering.

Is this still being worked on?

  • For my own use case, I'd want the additional option to limit headers to certain registries, e.g. for headers used for authentication
  • If official support were to be reintroduced, maybe [//my.registry.com/path:headers] to override or merge the default headers override if the user's registry is registry = http://my.registry.com/path. See other notes in [RRFC] Support sending custom request headers to an npm registry ($MY_AUTH_HEADER: $VALUE) #645 that I'd made before realizing this PR was open

@TysonAndre
Copy link

TysonAndre commented Oct 13, 2022

It seems like npm 9 will be dropping support for global configs entirely for _auth, _authToken, always-auth, etc, and only allowing those configs to be scoped to a given registry.

e.g. https://github.com/npm/cli/pull/5696

So maybe there should be no global [headers] section in an existing or new RFC to do this, and only a section such as this:

[//my.registry.com/my/prefix/path:headers]
my-xyz = xyz
[//registry.npmjs.org/:headers]
something-else = xyz

The last rfc meeting was in August - was that because there had been nothing to discuss, or another reason? https://github.com/npm/rfcs/issues?q=is%3Aissue+sort%3Aupdated-desc+is%3Aclosed+label%3Ameeting (I'm unfamiliar with the rfc process and didn't see anything about it in the last meeting notes)

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

Successfully merging this pull request may close these issues.

8 participants