feat: rewrite to pure js

[bxb100]

as mentioned in #80, this is a PR for rewriting to js

PS. I can't pass the default lint config, so I choose to use the GitHub action template eslint.yml

I tested Service Accounts and 1Password Connect locally using ⁠npm run local-action. If any reviewer is interested, you can test it locally by copying ⁠.env.example to ⁠.env.

[Xuanwo]

Thank you, @bxb100, for creating this; it's really great!

One suggestion I have is to add a CI check to verify that dist/core_bg.wasm and dist/index.js haven't been changed. I understand they are generated by js tools, and I trust all of you (@bxb100 and 1Password team members). However, it always makes me a bit concerned that someone might change the dist files by mistake. By adding CI checks, we ensure that the dist files are exactly what we expect to see and haven't been altered.

[Xuanwo]

NOTE to other reviewers: This PR is large due to the generated dist files; however, the significant changes are quite minimal.

[bxb100]

@Xuanwo Thank you for the review. I have learned a lot from your comments about the CI check. I have added a new workflow, ⁠check-dist.yml, to ensure the generated dist is reliable.

[Xuanwo]

@Xuanwo Thank you for the review. I have learned a lot from your comments about the CI check. I have added a new workflow, ⁠check-dist.yml, to ensure the generated dist is reliable.

Thank you, mostly LGTM now!

[Xuanwo]

Inviting @edif2008 and @SimonBarendse for a review.

[SimonBarendse]

Hi folks, really appreciate your efforts on this! ❤️ 🙌 Definitely aligned with the direction we'd like to move in for this and other integrations. We've build SDKs exactly because we saw such large desire from all of you to build integrations, and friction from doing so with CLI (e.g. performance, support for self-hosted runners, Windows support and it removes complexity from the implementation, making it easier to maintain and less error-prone). Excited to see you already help us move in that direction! 🚀

A couple things we'll want to look at to get this merged and deployed:

• Backwards compatibility: Continuity is quite important to us. We realize many depend on this and other integrations in critical workflows, and want to avoid disruptions as much as possible. With that in mind, we'll want to have a look at whether we can find a path to keep this action backwards compatible and at v2. So far, desktop app integration is the only missing feature in SDK we've identified to make a fully backwards compatible migration possible. While we work on closing that gap in SDKs, to not block this work from moving forward and make those aforementioned improvements available when not running locally (probably most uses), shall we support both side-by-side for now? We can check for Service Account token and/or Connect token envvars to use the new SDKs, and fall back to requiring CLI when those are not present (and thus probably running local). Once SDKs support Auth Prompts (aka desktop app integration) we can fully move over to SDK-only solution. That'd be a fully backwards compatible migration and we could stick to v2 throughout.
• Dist: Not sure why this action is using dist. @edif2008 is currently out, but when he returns next week, would love to learn from him what our reasoning was for introducing and if we could move away from that now. This could remove the complexity of required checks and prevent this repo from bloating by checking in a new binary for the WASM build on every new SDK release. Binary is already 5MB right now, and will likely grow further when expanding SDKs with additional functionality. With frequent SDK releases can add up quite quickly.
• Atomic Commits / PRs: To make the changes easier to review and move through faster, could you separate out unrelated changes in separate commits and PRs? For example I believe the changes to CONTRIBUTING.md, integration rename, linting changes, .gitignore additions, TS Config changes and .env.example introduction could each go in separate PRs, unless there's a relation that I'm not understanding yet. In that case, could you explain these changes and how they relate to this rewrite to help us understand and review? Shout-out to @AleksandrHovhannisyan for writing a great blog on this topic: https://www.aleksandrhovhannisyan.com/blog/atomic-git-commits/ 🙌

[Xuanwo]

Thank you @SimonBarendse for the bravo comments!

@bxb100 led the entire effort, so I will leave the final decision to him on how to implement changes. I'm here to explain why we need dist/.

For every uses: actions/checkout@v4, the GitHub Actions runner will clone the specified action actions/checkout and directly execute its entrypoint. There is no build or compile setup involved, so each action must include a bundled index.js file for the action to function properly.

The entry point is defined here:

load-secrets-action/action.yml

Lines 14 to 16 in 0a30992

	runs:
	using: "node20"
	main: "dist/index.js"

Additional documentation can be found here: https://docs.github.com/en/actions/sharing-automations/creating-actions/creating-a-javascript-action

[yafanasiev]

I am not sure if there are some dependencies in 1Password SDK that require ncc and wasm specifically, but I've had a great experience bundling JS GitHub Actions with esbuild. It's fast, supports tree shaking and the final bundle is smaller than NCC one.

[Xuanwo]

I am not sure if there are some dependencies in 1Password SDK that require ncc and wasm specifically

I'm guessing 1Password has written their logic in Rust, with the JS SDK being just a wrapper.

[SimonBarendse]

I'm guessing 1Password has written their logic in Rust, with the JS SDK being just a wrapper.

Yes, this is exactly it. We're using WASM to share a single implementation (in Rust) between all languages we support SDKs in. Right now that's Go, Python and JS, and we've built it with the intention to grow to more languages (in a scalable & sustainable way).

[SimonBarendse]

Thanks for the explanation on why dist is required @Xuanwo ! ❤️ That makes sense.

[bxb100]

update 11/23: force update with minimal changes. the original version lives in branch #82-bak

---

Thank you @Xuanwo, @SimonBarendse, and @yafanasiev for your reviews and discussions. I would like to provide some explanations and background information regarding the submitted content in this PR:

• .env.example is an example config used for local-action, which facilitates local testing directly.
• .eslintignore, .github/linters/.eslintrc.yml, package.json (deleting eslintConfig) are changes made because I couldn't use the original @1password/front-end-style/eslintrc.yml when submitting the code. Therefore, I switched to the configuration recommended by https://github.com/actions/typescript-action.
• __tests__, config/jest.config.js are to expand the original test set.
• .github/workflows/check-dist.yml, .node-version are used to check the consistency of the generated dist for every PR and PUSH as mentioned before. It requires executing npm run bundle, which includes format:write.
• CONTRIBUTING.md results from the pre-push hook from husky executing npm run validate(contain format). I apologize for its appearance in this submission.
• The changes to .github/linters/tsconfig.json and tsconfig.json will be reverted.
• Regarding the fallback to CLI when there are no environment variables, perhaps we can follow @Xuanwo 's suggestion and bump into v3, then iterate gradually. I understand this occurs mainly during local testing, but it can also be bypassed by setting the environment variables.
• About compatibility, the inputs in action.yml remain consistent with version v2. The environment variables OP_SERVICE_ACCOUNT_TOKEN, OP_CONNECT_HOST, and OP_CONNECT_TOKEN also remain unchanged.
• Based on my understanding of action uses ¹, I believe frequently changing the dist output will only affect users who do not use the major version and need to update manually (although, to be honest, I haven't seen this usage scenario).
• For the bundled submission of this PR, since it was converted from one of my projects ², I did not consider atomic submissions. I sincerely apologize for any inconvenience this may have caused.

Footnotes

1. https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsuses ↩

2. https://github.com/bxb100/load-secrets-action ↩

[Xuanwo]

Backwards compatibility

Hi @SimonBarendse, I suggest we avoid taking this direction since switching from CLI-based to JS-based does introduce a breaking change. I can foresee multiple ways this action could fail. It's common and expected for GitHub Actions users to upgrade actions between major versions (every Node version bump will trigger one).

Adopting load-secrets-action@v3 would provide a much smoother transition for the community. Releasing v3 doesn't mean users will adopt it by default; they will continue using v2 unless they manually update their workflows.

I am willing to create a migration guide for users to follow if there are any break happened.

• For the bundled submission of this PR, since it was converted from one of my projects 2, I did not consider atomic submissions. I sincerely apologize for any inconvenience this may have caused.

Hi @bxb100, I understand the concern from @SimonBarendse's side. To make the PR review smoother, do you think it would be a good idea to simply remove all files except for package.json, src/, and dist/? We can gradually add other staffs back later (which makes it much easier to review without involving real logic).

[SimonBarendse]

I would like to provide some explanations and background information regarding the submitted content in this PR

Thank you @bxb100! 🙌 This is very helpful.

Followed up in separate issues

• ES Lint Config: I've opened [Tooling] ES Lint Config does not work for Open Source #83 for us to follow up on this.

• Linting:

  CONTRIBUTING.md results from the pre-push hook from husky executing npm run validate(contain format). I apologize for its appearance in this submission.

  Thank you! This uncovered a gap in our linting in CI. We're following up in Check formatting in CI/CD pipeline #84

Rewrite specific changes - Discussing in this PR

Backwards Compatibility

I suggest we avoid taking this direction since switching from CLI-based to JS-based does introduce a breaking change. I can foresee multiple ways this action could fail.

@Xuanwo could you help me see these too? I'm currently under the impression that the new version of this action built on SDK will be fully compatible with the current version built on CLI, after addition of Auth Prompts. As @bxb100 mentioned as well:

the inputs in action.yml remain consistent with version v2. The environment variables OP_SERVICE_ACCOUNT_TOKEN, OP_CONNECT_HOST, and OP_CONNECT_TOKEN also remain unchanged.

Could you please help me see and understand what I'm missing? What are the failures you foresee?

Dist

Yes, I'm less concerned about this now and okay with proceeding with checking in the WASM binary into the code here after @Xuanwo's explanation why this is necessary. Apologies that I not explicitly mentioned that before.

I assume and hope when using the action that GitHub only fetches the referenced commit and not the full Git history, similar to the checkout action, by using --depth=1. That prevents historic binaries slowing down the action and increasing required bandwidth.

So the larger repo size from historic binaries checked in would only affect new contributors checking out the full repo, including all historic WASM binaries. But it won't affect using the action.

With that in mind, I believe we can pull back in the changes to dist and your introduction of .github/workflows/check-dist.yml.

Code Review

I've requested a review from @Marton6 and @edif2008 for the specific code changes made in this PR.

[bxb100]

Thank you, @SimonBarendse, for the detailed explanation.

With that in mind, I believe we can pull back in the changes to dist and your introduction of .github/workflows/check-dist.yml.

I believe we can proceed after #83 #84 .

I assume and hope when using the action that GitHub only fetches the referenced commit and not the full Git history, similar to the checkout action, by using --depth=1. That prevents historic binaries slowing down the action and increasing required bandwidth.

base on https://github.com/actions/runner/blob/078eb3b381939ee6665f545234e1dca5ed07da84/src/Runner.Worker/ActionManager.cs#L784C1-L792C1 I think it direct download like https://api.github.com/repos/{action.NameWithOwner}/tarball/{action.Ref}

[edif2008]

Hey @bxb100!

First of all, thank you for doing all of this rewrite. I really appreciate it and it looks great so far.

A couple of pieces that I wanted to raise here:

1. Backwards compatibility
   I've re-read the code for the current version of this GitHub Action and the desktop app integration has never been an authentication method supported by this action. It will error if neither of the connect nor service account environment variables are configured. This should, therefore, smoothen this migration to a pure JS action and significantly decrease the risk of this not being backwards compatible.

2. Windows support
   Since with this change we can (finally) add support for Windows runners, would you mind adding this OS in the test.yml file in the os matrix? It should look like this:

   strategy:
     matrix:
       os: [ubuntu-latest, macos-latest, windows-latest]
       auth: [connect, service-account]
       exclude:
         - os: macos-latest
           auth: connect
         - os: windows-latest
           auth: connect

3. Code review
   The overall approach of the changes in this PR look really good. A couple of them are nitpicks and improvements in naming and such, but a couple of key areas to look at are:

   • erroring when duplicate fields are found.
   • identifying environment variables that have secret references and validating them.
   • simplifying the code for validating the authentication and building the client that will resolve the secret references.

[edif2008]

Hey @bxb100!
PR #100 has been merged. Would you be kind to bring those changes to your branch? Then we can run the tests on this PR and see how it goes. 😄
And thank you for your patience on getting this to work.

[bxb100]

@edif2008 done, PTAL

[edif2008]

/ok-to-test sha=6cdba15
Reference workflow

[edif2008]

Hey @bxb100!
Based on the executed workflow, it seems that the tests still fail with the following error:

The item has more than one 'password' field

Could it be that you forgot to update the dist directory based on the latest changes? If so, could you do that to see if there are any other things missing from a functionality standpoint?

[bxb100]

@edif2008 sry for this, already done, PTAL

[edif2008]

/ok-to-test sha=b6ea213
Reference workflow

[edif2008]

Hey @bxb100!

Based on the latest tests run I've identified the following regressions:

• Looking for archived items when resolving secret references (only in SDK). If you have 2 items in a vault, one active and one archived, resolving a secret reference will fail with the following error:

  error resolving secret reference: more than one item matched the secret reference query
  

  This is something that I've reported internally and we're looking into it.

• Resolving references that are unique to the item with duplicated sections (both in SDK and Connect secret reference parsing).
  If we have an item with two sections like so:

  Example item
  |-- duplicate-section
  |   |-- foo
  |-- duplicate-section
      | -- bar
  

  and we want to resolve the following secret reference: op://vault/Example item/duplicate-section/foo, the CLI will successfully resolve it. However, the SDK and the Connect function resolveByPath, we will get the following error:

  error resolving secret reference: more than one section matched the secret referenc
  

  This regression doesn't block this PR itself (since at least it behaves the same for both types of clients). However, it increases the list of missing pieces for making the rewritten action on par with the CLI version of it. We've filed this internally for now.

There's no action for you at the moment unfortunately. I'm currently re-reviewing the PR itself from a code perspective to ensure all the pieces are in a good shape and then will let you know on the next steps. Once that's done, I think we're good to go with this rewrite.
I also want to let you know that I intend to merge this into a new branch named v2-alpha to not block the main branch from fixing bugs and other things in the meantime.

[edif2008]

Code review: ✅

Things look great now and I feel more confident on this version of the rewrite. This PR is good to go acknowledging the following regressions:

• Both clients
  • Successfully resolving a secret reference when the item has duplicate sections, but the field is unique in the entire item.
• SDK Client only
  • Successfully resolving a secret reference when there's a duplicate item that is archived.
  • Query parameters - only attribute=otp/totp and ssh-format are supported at the moment in the SDKs.
  • File support
  • SSH key item support

The last 2 regression points can be addressed by upgrading the SDK from 0.1.7 to 0.2.0. The second regression point (related to attribute in the reference) will support SSH key format now, but nothing else. I will leave it up to you whether you want to upgrade to 0.2.0 as part of this PR or not.

Other notes

When checking this branch, npm was telling me the following:

2 vulnerabilities (1 moderate, 1 high)

To address all issues, run:
  npm audit fix

Therefore, it might be worth running npm audit fix on your branch, update the dist directory and push those changes.

[bxb100]

@edif2008 Thanks for your reviewing, all done, PTAL

[edif2008]

/ok-to-test sha=417a5bb
Reference workflow

[edif2008]

Things look great in this MR now. Thank you so much for the patience and the great effort in making this rewrite! 💪🏻

[edif2008]

Hey @bxb100!

I have two things to raise with you:

1. We've just released the SDK 0.2.1, which resolves some of the regressions that we've identified. Do you have the time to update to the latest SDK and update the Connect client to behave the same as the SDK when it comes to the following scenario?

   Successfully resolve a secret reference when the item has duplicate sections, but the field is unique in the entire item.

2. We've decided to hold this PR from being merged until the last missing piece is implemented in the SDK so that we can confidently merge the complete rewrite of the action. Specifically, this one:

   Query parameters - only attribute=otp/totp and ssh-format are supported at the moment in the SDKs.

[bxb100]

As a 1Password user, I’m thrilled to see your team introducing so many new features. However, I’ve noticed that the update cycles for ⁠onepassword-sdk-js and ⁠connect-sdk are not aligned. This inconsistency, combined with my limited understanding of certain features in these SDKs, has made rewriting actions more challenging.

Additionally, the recent supply chain attack involving ⁠tj-actions/changed-files has heightened my concerns about the security risks of using JavaScript to rewrite actions, as it could inadvertently introduce vulnerabilities to the community.

Given these concerns, I’d like to propose reconsidering a CLI-based approach instead. For example, something like KamaranL/1password-load-secrets-action might be a viable alternative.

@edif2008 @Xuanwo @SimonBarendse @yafanasiev I sincerely apologize for the time and effort spent reviewing this code, and I appreciate your understanding.

Thank you!

[Xuanwo]

Thank you so much @bxb100 for working on this.

I respect his decision to stop working on this, but I still believe implementing actions in pure JS with a well-maintained codebase is a good direction and benefits the overall community.

cc @edif2008, This PR is almost ready, and we now have a solid starting point. I suggest merging it into v2-alpha first and then refining it further.

Some concerns raised by @bxb100 are valid. I believe the 1Password team should conduct a thorough audit of all actions and SDK dependencies to prevent such issues from occurring.

[Xuanwo]

This PR also teaches us about the collaboration methods between open-source contributors and maintainers. Both of our teams put their best efforts and kindness into this PR. We even got it approved and nearly ready to be merged, but it ended up being closed.

I can relate to @bxb100's feelings of complete burnout after working on this three-month-long PR, continuously receiving new review comments, and even being asked to implement entirely new features that never existed before.

Don't get me wrong—I really appreciate @SimonBarendse's and especially @edif2008's work on this PR as well. However, if we get another opportunity, maybe we could take a different approach—one that's more incremental and positive.

[SimonBarendse]

Additionally, the recent supply chain attack involving ⁠tj-actions/changed-files has heightened my concerns about the security risks of using JavaScript to rewrite actions, as it could inadvertently introduce vulnerabilities to the community.

My understanding of this attack is that a PAT from a bot account was leaked, then used to commit malicious code to the repo. I don't see how the language used for the action has any relation to this. Once a PAT is leaked that can push code, that's a problem regardless of the language used for the action. Could you help me understand how using JavaScript would be different?

I believe preventing these attacks comes down to protecting all credentials that allow pushing code. We currently have these protections in place for this:

1. We're protecting the PATs from the automations/bot (1PasswordSDKBot) that push code to the GitHub repos for the SDKs by storing and loading them through 1Password.
2. The PATs of automations/bots do not have permission to push to main. Every code change, including changes pushed by the bot, needs to go through a PR and reviewed and approved by a 1Password employee (example for the latest JS SDK release).
3. There are no personal access tokens that have access to commit to this 1password/load-secrets-action repo. Same as the SDK repos, all code requires review by a 1Password employee before being merged to main.

If you have any additional recommendations, feel free to share them.

This PR also teaches us about the collaboration methods between open-source contributors and maintainers. Both of our teams put their best efforts and kindness into this PR. We even got it approved and nearly ready to be merged, but it ended up being closed.

Thank you @Xuanwo ! Fully agreed that we're learning a lot from this PR and improving the collaboration and experience for contributors. We're keen to enable the open source community, and appreciate all you've both done to enhance the experience, including providing this and prior feedback! I just gave this MR a quick scan, and see 5 improvements to the contributor experience made b/o learnings from this PR:

1. Fixed CI linting and tests for external contributions [Tooling] ES Lint Config does not work for Open Source #83, Make workflow targets more specific #87, Run acceptance tests on fork #97
2. Expanded linting with more checks Check formatting in CI/CD pipeline #84, Add lint to workflow #88

I really appreciate you helping us uncover these, and believe this will create a better experience for next contributors! 💙

I can relate to @bxb100's feelings of complete burnout after working on this three-month-long PR, continuously receiving new review comments, and even being asked to implement entirely new features that never existed before.

First off: I'm sorry you haven't had a good experience working on this PR. Definitely not our intention that this is burning you out.

And I agree that there's been more effort than we had originally anticipated. There's been the hurdles you've ran into as first external contributor to this repo that I mentioned above, and additionally, I believe we had overestimated how far along the new version 0 1Password SDKs already were in achieving parity with 1Password CLI.
I think there may have been a miscommunication about expectations we have: we do not expect you to fill the gaps in parity between the SDK and CLI. That's currently not even possible yet, because the core logic of the SDKs has not (yet) been open sourced. Filling those gaps is a dependency in merging this PR though, because we want to avoid breaking users workflows.
I believe it's very fair feedback that new challenges were uncovered in subsequent reviews, that could have been uncovered sooner. Especially knowing SDKs are in version 0, we could have done a more holistic assessment of what were the missing features for CLI compatibility earlier on, to prevent surprises later, after you had already spent more time on this PR.

I still believe implementing actions in pure JS with a well-maintained codebase is a good direction and benefits the overall community.

I fully agree. This is also still in full alignment with the direction we're heading across all integrations (see earlier comment). Migrating integrations from CLI to SDKs is still something we want to pursue as a team.
We now know what is needed on SDKs first to achieve parity, and after we tackle that we can revisit this work. It's fully up to you if you want to be involved in that, we welcome you for sure, and appreciate the collaboration we've had so far! If you prefer we or someone else takes over getting this across the finish line (when it reaches the top of our backlog) that's okay too, and I just want to thank you very much for all the work so far! 💙