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

Lacking null checks and improved method description and running tests #339

Closed
christian3042 opened this issue Feb 20, 2025 · 3 comments · Fixed by #340 or #342
Closed

Lacking null checks and improved method description and running tests #339

christian3042 opened this issue Feb 20, 2025 · 3 comments · Fixed by #340 or #342
Assignees
Labels

Comments

@christian3042
Copy link

christian3042 commented Feb 20, 2025

Describe the bug

During the development of an application using the ngx-cookie-service, I ran into some issue, that my Cookies didn't get deleted. Checking the Readme I found the solution:

Deletes all cookies that can currently be accessed. It is best practice to always define a path. If you are unsure about the path value, use '/'.

This should replace the description of the deleteAll() method, as

Delete all cookies

is confusing and can be misunderstood. Because the non accessible cookies from other paths won't get deleted.

I also checked the code and found some missing null checks, and tried to fix it:
/projects/ngx-cookie-service/src/lib/cookie.service.ts:

get(name: string): string {
    if (this.check(name)) {
      name = encodeURIComponent(name);

      const regExp: RegExp = CookieService.getCookieRegExp(name);
      const result: RegExpExecArray | null = regExp.exec(this.document.cookie);

      return result ? (result[1] ? CookieService.safeDecodeURIComponent(result[1]) : '') : '';
    } else {
      return '';
    }
  }

projects/ngx-cookie-service-ssr/src/lib/ssr-cookie.service.ts:

check(name: string): boolean {
    name = encodeURIComponent(name);
    const regExp: RegExp = SsrCookieService.getCookieRegExp(name);
    const cookie: string | null | undefined = this.documentIsAccessible ? this.document.cookie : this.request?.headers.get('cookie');
    return cookie ? regExp.test(cookie) : false;
  }
get(name: string): string {
    if (this.check(name)) {
      name = encodeURIComponent(name);
      const regExp: RegExp = SsrCookieService.getCookieRegExp(name);
      const cookie: string | null | undefined = this.documentIsAccessible ? this.document.cookie : this.request?.headers.get('cookie');
      const result: RegExpExecArray | null = cookie ? regExp.exec(cookie) : null;
      return result ? (result[1] ? SsrCookieService.safeDecodeURIComponent(result[1]) : '') : '';
    }
    return '';
  }

Unfortunately I didn't manage to run the tests on my machine. Is there any documentation on how to run the tests? I tried to use jest / ts-jest and use same babel / jest config.js but ran from one error to another. Could you also please provide the test dependencies in the package.json and also test configuration files? That would help other devs, to run the tests easily.

Steps to Reproduce

Linter errors are shown in VSCode:

Type 'RegExpExecArray | null' is not assignable to type 'RegExpExecArray'.
Type 'null' is not assignable to type 'RegExpExecArray'.ts(2322)

Expected behaviour

No linter errors.

What version of the library you see this issue?

19.0.0

What version of Angular are you using?

Angular 19

Copy link

Hello 👋 @christian3042
Thank you for raising an issue. We will investigate into the issue and get back to you as soon as possible. Please make sure you have given us as much context as possible.
Feel free to raise a PR if you can fix the issue

@pavankjadda
Copy link
Collaborator

@christian3042 null checks are not necessary, as it returns RegExp object

  private static getCookieRegExp(name: string): RegExp {
    const escapedName: string = name.replace(/([\[\]{}()|=;+?,.*^$])/gi, '\\$1');

    return new RegExp('(?:^' + escapedName + '|;\\s*' + escapedName + ')=(.*?)(?:;|$)', 'g');
  }

and updated descriptions

@christian3042
Copy link
Author

There was a similar issue already and it partly has been fixed: #122

The reason why null checks are necessary, is because regExp.exec(this.document.cookie) can return null. You can check the interface RegExp:

exec(string: string): RegExpExecArray | null;

The code itself executed might not return a null value in this case, but from the interface definition a null check should be done.
You can also check the code in VSCode and you will get the errors highlighted.

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