I'd say this is expected behaviour/misuse. Definitely not a security issued unless you can provide me with some real world examples about how an attacker could exploit this.

Connections are taken from the pool and then put back, depending on your using the pool/making requests (it's not clear here what that is as you've not provided any code) it is expected that changes you make to a connection will remain on that connection when it is returned to the pool and you must take care to ensure you're handling this appropriately.

If you're setting contexts on connections you need to make sure that you are resetting those before returning them to the pool.

Example, is provided by microsoft: "Scenario for users who connect to the database through a middle-tier application"
https://learn.microsoft.com/en-us/sql/relational-databases/security/row-level-security?view=sql-server-ver16

Due to this error and following Microsoft's practice, the system can randomly disclose data to users who are not supposed to view it.

Think of salaries or medical records. That is a security issue, and one that can get expensive.

The connection is the physical communication channel between SQL Server and the application: the TCP socket, the named pipe, the shared memory region. The session in SQL Server corresponds to the definition of a [session]: a semi-permanent container of state for an information exchange. (Wikipedia)

It's just wrong to not distinguish them.

Regarding the issue you have reported: as you haven't provided any example code, the support I can provide is limited. As I've said, you'll have to clear the session on the connection before returning it to the pool, that will resolve your problem as far as I can tell.

In terms of if this is a security issue: If you can't provide a proof-of-concept about how this is exploitable, then it's not a provable vulnerability and just an unfounded assertion.

If you're somehow adding context to a connection and then returning it to a pool and then pulling it from the pool again whilst not resetting the context and expecting it to magically now have some new context, that is misuse. The same way creating constructing a query in a way that is vulnerable to SQL injection is misuse and not a vulnerability of the library.

Just like SQL injection vulnerabilities, the fact you're not cleaning the connection context can be a security issue for your application but it's not a concern of the underlying library.

Now, if you're asking that when connections are returned or obtained from the pool that sp_reset_connection is called, then that's fair enough and is a feature request I'd be open to.

For reference, here is a similar issue for SQLAlchemy

You are seriously asking me to copy and paste the example from microsoft to this ticket?
https://learn.microsoft.com/en-us/sql/relational-databases/security/row-level-security?view=sql-server-ver16

It is the proof-of-concept.

SQLAlchemy is a different case, cause its the same process. In node.js with mssql it could be different processes and so different users sharing sessions. Which makes no sense. If this is by design, then the design is simple wrong. There is a reason for making a difference between session and connection.

You are seriously asking me to copy and paste the example from microsoft to this ticket?

No, I'm asking for a PoC for how this library is vulnerable to a security issue. Specifically one showing that it's a fundamental flaw in the library vs consumers of the library not cleaning up after themselves. That means Node code that shows a predictable and reproducible problem and a rational as to why it's a flaw with the library and not user error.

SQLAlchemy is a different case

Yes, I said similar issue, not same. It's about session data being cleaned from the connection when releasing connections from a pool.

In node.js with mssql it could be different processes and so different users sharing sessions

Incorrect, an sql pool is not shared across processes. It may be shared across requests but Node is single process and single thread, not multi-process, so the pool is not shared across processes. Each instance of a node process will have its own pool.

[sharing database sessions across user sessions] makes no sense. If this is by design, then the design is simple wrong. There is a reason for making a difference between session and connection.

I think it depends on the perspective; however I would agree that most of the time you want to get a "clean" connection from the pool. I'm not disagreeing there and, as I said, happy to entertain the idea of cleaning the connection when it's released. What I'm saying is this is not a security issue.

Then please add this behavior to the documentation. That not only the connection is pooled, also the session and any may added data is.

Incorrect, an sql pool is not shared across processes. It may be shared across requests but Node is single process and single thread, not multi-process, so the pool is not shared across processes. Each instance of a node process will have its own pool.

What about when people use Workers?

As far as this library and how it's implemented, it is not possible to share a pool between workers.

I believe it is theoretically possible to share sockets between parent and child workers, so a child worker could in theory share an sql connection, but that's well outside the scope of the library.

If the pool of connections can keep sessions open, does that also mean that session handling set statements can also be carried over?

SET DEADLOCK_PRIORITY LOW, SET IDENTITY_INSERT ON or SET NOCOUNT ON?

If so does this mean we should set these when obtaining a new connection from the pool?

A connection is returned to the pool in the exact state it is in when it is no longer being used by the application.

This means these statements will stay with the connection, yes, but you can't make any assumptions that the next time you retrieve a connection from the pool that it will be in a particular state if you have no ensured this in the application layer.

The library does not do any "magic" if you put a connection back in the pool in a particular state, that is how it will remain until it is closed or cleaned-up.

This means these statements will stay with the connection, yes,

I tried this:

for (let i = 0; i < 10; ++i)
{
	let ret1;
	if (i === 0)
	{
		ret1 = await mssql.query`set nocount on;select 1;`;
	}
	else
	{
		ret1 = await mssql.query`select 1;`;
	}
	console.log(ret1.rowsAffected);
}

Only the first output is [], the rest are [1]. So the set nocount on status doesn't stay with the connection.

If this can stay, what about declare a variable? Is it remain available when the connection reuse again?

@heroboy that depends how many connections are in your pool and whether you're ever getting the same connection out again.

I wouldn't be relying on the connections retaining state because you have no way to know if the connection you use for one query will be the same as the one you use for the next.

@dhensby

that depends how many connections are in your pool and whether you're ever getting the same connection out again.

That's why I wrote a for loop that runs 10 times—eventually, there would always be a connection object that inherits the state when i == 0. However, in reality, not a single instance of SET NOCOUNT ON was inherited.

I'm not relying on the connection retaining state, but I do need to know which states are preserved and which are reset. For the states that aren't reset, I need to explicitly set them to the desired values each time I obtain a new connection object.

For example, if the SET NOCOUNT ON state were preserved, and my current SQL statement needs to use rowsAffected, then I would have to include SET NOCOUNT OFF in this SQL statement.

There are too many states in SQL for me to be familiar with all of them. If the connection object retrieved from the connection pool isn't "clean" enough and causes unpredictable effects on the SQL statements being executed, that would be quite troubling for me.

Actually, I care quite a lot about this issue. I've come across sp_reset_connection and also seen related protocol handling in tedious, but I still don't really understand the correct way to approach it. I've thought about how to implement a proper connection pool myself, whether in Node.js or in ADO (the traditional COM-based ADO, not ADO.NET).

I think for this library, it would be sufficient if you could handle it to the same level as ADO.NET.

I've done some investigation and looked into this when working directly with tedious and I get exactly the same behaviour, ie: set nocount on doesn't seem to persist on a connection.

After some digging I believe I've been able to get to the bottom of it. Under the hood, when tedious is executing SQL it uses sp_executesql, this is for performance reasons, and as such the connection settings like set nocount are not set against the connection, just the query being executed.

It is possible to get around this by using the execSqlBatch method instead (which does not make use of sp_executesql) and is accessible using this library's batch method instead of query.

eg: await mssql.batch`set nocount on`;

This still won't solve the problem that you're not guaranteed to get a connection back that has had set nocount on run on it.

tedious does have the internal concept of initial sql, but there's no way to customise it and there's no support for setting nocount at the moment (though I'm sure a PR could put a change to that).

Please note in the connection.reset function:

https://github.com/tediousjs/tedious/blob/ebb023ed90969a7ec0e4b036533ad52739d921f7/src/connection.ts#L3245-L3254

It set resetConnectionOnNextRequest = true. It is a flag related to the TDS protocol (defined in packet.ts). Although I’m not exactly sure what they do, it appears that it also serve to reset certain states.

You can search RESETCONNECTION in TDS Protocol https://winprotocoldocs-bhdugrdyduf5h2e4.b02.azurefd.net/MS-TDS/%5bMS-TDS%5d.pdf
search _fResetConnection, ST_RESET_CONNECTION in https://github.com/dotnet/SqlClient

And again, I don't know the detail.

@heroboy yes- it's the equivalent of calling sp_reset_connection and clears all settings on the connection. The initial issue raises that this is something that should be done for pooled connections.

After some testing, I found that calling connection.reset before executing a new request seems to solve the problem.
However, I’m not sure if using connection.reset in this way could introduce other side effects or break existing behavior