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.

Sign up for GitHub

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

Jump to bottom

v5 #450

Merged
merged 22 commits into from
Sep 23, 2023
Merged

v5 #450

merged 22 commits into from
Sep 23, 2023

Conversation

DoctorMcKay
Copy link
Owner

@DoctorMcKay DoctorMcKay commented Jul 25, 2023 •
edited
Loading

Migration Guide

Breaking

  • Now requires Node.js v14 or later (up from v8)
  • Removed deprecated methods
    • setSentry()
  • Removed deprecated events
    • sentry
  • Removed deprecated login key functionality
    • Removed logOn properties: loginKey and rememberPassword
  • Removed legacy authentication

Other Changes

  • Added refreshToken event
  • Added renewRefreshTokens option

To Do

  • Remove setSentry()
  • Remove sentry event
  • Remove login key functionality
  • Remove legacy auth
  • Add jsdoc to logOn
  • Probably add some event to access refresh tokens
  • Refresh token renewal

@Sadzurami
Copy link
Contributor

Sadzurami commented Jul 25, 2023 •
edited
Loading

What do you think about moving to TypeScript? Is this possible in future versions?
Maybe it helps with "awful inheritance" that is being used right now.

@DoctorMcKay
Copy link
Owner Author

I do intend to move to TypeScript at some point, but it shouldn't be a breaking change so no need to rush it here.

Sadly I don't think it's going to help with the inheritance madness since I'm not aware of any way to split a class into multiple files using TS either.

v5 is kind of an expedited thing since protobufjs has a significant security vulnerability which is only fixed in protobufjs v7, and updating that dependency is a breaking change since it drops support for node 8-12.

@Sadzurami
Copy link
Contributor

Sadly I don't think it's going to help with the inheritance madness since I'm not aware of any way to split a class into multiple files using TS either.

Am i right, the only problem with inheritance - intellisense that not picking up on methods?
If it is true, "declare" in typescript can be useful to solve this problem.

@DoctorMcKay
Copy link
Owner Author

Am i right, the only problem with inheritance - intellisense that not picking up on methods? If it is true, "declare" in typescript can be useful to solve this problem.

Perhaps that works, though I'd have to go back and change everything back to using the old prototype syntax, which I don't think is really desirable.

@DoctorMcKay DoctorMcKay mentioned this pull request Jul 28, 2023
Closed
@DoctorMcKay DoctorMcKay linked an issue Jul 28, 2023 that may be closed by this pull request
[SECURITY] Security issues with package dependencies #452
Closed
Sadzurami
Sadzurami reviewed Aug 13, 2023
View reviewed changes
components/07-web.js

let transport = new CMAuthTransport(this);
let session = new LoginSession(EAuthTokenPlatformType.SteamClient, {transport});
let session = this._getLoginSession();
session.refreshToken = this._logOnDetails.access_token;
session.getWebCookies().then((cookies) => {
if (!cookies.some(c => c.startsWith('sessionid='))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

since version 1.3.0 node-steam-session always return sessionid

Copy link
Owner Author

Choose a reason for hiding this comment

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

Doesn't hurt anything to double check.

@Sadzurami
Copy link
Contributor

Looks like all todo's complete.
When can we expect an update?

@DoctorMcKay
Copy link
Owner Author

Looks like all todo's complete. When can we expect an update?

I want to test it a little more. If you want to install from the branch and test it yourself with your code, that would help.

@Sadzurami
Copy link
Contributor

Sadzurami commented Aug 24, 2023 •
edited
Loading

Looks like all todo's complete. When can we expect an update?

I want to test it a little more. If you want to install from the branch and test it yourself with your code, that would help.

I tested v5 a few days after your last comment.
Tested it on some projects with many instances at the same time.

Looks pretty stable, no issues found.

But I don't test new features like renewRefreshTokens.
Used version 5 only as drop-in replacement for version 4.

Tested on node v20

@DoctorMcKay DoctorMcKay marked this pull request as ready for review September 23, 2023 03:48
@DoctorMcKay DoctorMcKay merged commit 93063d7 into master Sep 23, 2023
@DoctorMcKay DoctorMcKay deleted the v5 branch September 23, 2023 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sadzurami Sadzurami Sadzurami approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SECURITY] Security issues with package dependencies
2 participants
@DoctorMcKay @Sadzurami